Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable zend.assertions=1 by default #582

Closed
2 of 5 tasks
Seldaek opened this issue Apr 1, 2022 · 3 comments
Closed
2 of 5 tasks

Enable zend.assertions=1 by default #582

Seldaek opened this issue Apr 1, 2022 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@Seldaek
Copy link

Seldaek commented Apr 1, 2022

Describe the bug
It appears zend.assertions is set to 0 or -1 by default with setup-php.

As you can see on this build I messed up because I didn't have assertions enabled locally so I didn't realize I had broken assertions, and when I pushed only some builds failed, which was even weirder.

Per https://www.php.net/manual/en/function.assert.php you can see the recommended development value is 1.

Version

  • I have checked releases, and the bug exists in the latest patch version of v1 or v2.
  • v2
  • v1

Runners

  • GitHub Hosted
  • Self Hosted

Operating systems
ALL

PHP versions

7+

To Reproduce

<?php assert(false);

Expected behavior

It should throw but it doesn't on php7+

Screenshots/Logs

https://github.com/Seldaek/jsonlint/runs/5788312601

Additional context

Are you willing to submit a PR?

@Seldaek Seldaek added the bug Something isn't working label Apr 1, 2022
@shivammathur
Copy link
Owner

@Seldaek
setup-php uses production php.ini in v2 as it was the default in the builds. I plan to change to the development one in v3.

For now you can specify the ini-file input to development or set zend.assertions=1 in the ini-values input.

@Seldaek
Copy link
Author

Seldaek commented Apr 1, 2022

OK I see, feel free to close then, looking forward to v3 :) It's not a huge deal so I rather not edit all my workflows for this, as long as it's on your radar 👍🏻

@mvorisek
Copy link

I support this change. setup-php is used for CI testing a lot and only very few users set production mode or zend.assertions php.ini manually.

Having zend.assertions enabled by default will imply performance negligibly but having them disabled (which is the case now) is making a lot of people think they have their assert() directives tested.

Also, per https://www.php.net/manual/en/function.assert.php the zend.assertions is enabled by default when no production/development flavoured php.ini is used. This is quite important fact, as because of this official Docker images https://github.com/docker-library/php have zend.assertions enabled. So setup-php should have zend.assertions enabled as default as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants