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.

Sign up for GitHub

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

Jump to bottom

Expose and fix incorrect assertion #463

Merged
merged 2 commits into from
Feb 22, 2022
Merged

Expose and fix incorrect assertion #463

merged 2 commits into from
Feb 22, 2022

Conversation

agustingomes
Copy link
Contributor

This change intends to fix a fatal error happening when zend.assertions=1 is defined in the INI settings

@agustingomes
Copy link
Contributor Author

Hi @lcobucci

When upgrading to the 7.2.0 release I came across some fatal errors, which I was able to trace down to the fact the Docker image I was using had zend.assertions=1 set in the INI configuration, and upon checking the CI setup, I confirmed that by this config value being absent, the test was reported as passing incorrectly.

The actions already ran in my Repository fork, which allowed me to confirm the issue: https://github.com/agustingomes/lcobucci-di-builder/runs/5293755445?check_suite_focus=true

Let me know If I can assist you further into resolving this issue.

@lcobucci lcobucci self-assigned this Feb 22, 2022
@lcobucci lcobucci added the Bug label Feb 22, 2022
@lcobucci lcobucci added this to the 7.2.1 milestone Feb 22, 2022
@lcobucci lcobucci changed the base branch from 7.3.x to 7.2.x February 22, 2022 19:48
@lcobucci
Copy link
Owner

Ouch, that's not okay!
This is definitely a bug so I sent it to the 7.2.x branch.
Would you mind rebasing it?

@lcobucci
Copy link
Owner

It would be nice to have this in the Makefile

@agustingomes agustingomes force-pushed the zend-assert-issue branch 2 times, most recently from 9eca207 to ae352d6 Compare February 22, 2022 19:52
@agustingomes
Copy link
Contributor Author

agustingomes commented Feb 22, 2022 •
edited
Loading

Added the setting to the phpunit target and rebased the branch onto 7.2.x

@lcobucci lcobucci changed the title Run PHPUnit with Zend assert enabled Expose and fix incorrect assertion Feb 22, 2022
@lcobucci lcobucci merged commit f260e88 into lcobucci:7.2.x Feb 22, 2022
@lcobucci
Copy link
Owner

Thanks @agustingomes !

@github-actions github-actions bot mentioned this pull request Feb 22, 2022
Merged
@agustingomes
Copy link
Contributor Author

Anytime @lcobucci 👍🏼

@agustingomes agustingomes deleted the zend-assert-issue branch February 22, 2022 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcobucci lcobucci lcobucci left review comments

Assignees

@lcobucci lcobucci

Labels
Bug
Projects
None yet
Milestone
7.2.1
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@agustingomes @lcobucci