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

Use PHP_VERSION_ID to check the minimum required version #3453

Merged
merged 2 commits into from
Mar 25, 2016
Merged

Use PHP_VERSION_ID to check the minimum required version #3453

merged 2 commits into from
Mar 25, 2016

Conversation

unfunco
Copy link
Member

@unfunco unfunco commented Feb 17, 2016

This is a micro-optimisation, but it avoids the overhead of two function calls to determine whether the minimum required version of PHP is present which happens on each request.

I have also removed the conditional around the a call to gc_collect_cycles within the tests since it checks for version 5.3+, and the minimum version required to run Magento 2 should already satisfy that condition.

This is a micro-optimisation, but it avoids the overhead of two function
calls to determine whether the minimum required version of PHP is present.

I have also removed the conditional around the a call to gc_collect_cycles
within the tests since it checks for version 5.3+, and the minimum version
required to run Magento 2 should already satisfy that condition.
@mazhalai
Copy link
Contributor

mazhalai commented Mar 8, 2016

Internal ticket MAGETWO-50222

@mazhalai mazhalai added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label Mar 8, 2016
@ark99
Copy link
Contributor

ark99 commented Mar 16, 2016

@unfunco, PHP_VERSION_ID was introduced in 5.2.7 (http://php.net/manual/en/reserved.constants.php). What if the php version is older than that? We may want to check if PHP_VERSION_ID constant is defined first, If not fall back to using phpversion().

So now we are doing two checks and sort of lose the benefits of micro-optimization. If you agree, we should consider leaving the original logic as it is.

Thanks!

@davidalger
Copy link
Member

@ark99 If that is a concern, what about doing this…

if (!defined('PHP_VERSION_ID') || PHP_VERSION_ID < 50500) {
    // ...
}

...OR...

if (@PHP_VERSION_ID < 50500) {
    // ...
}

The later would be one of the few use cases where using the error suppression operator is allowable, as it's effect is only to allow the successful reporting of an invalid PHP version for really old software stacks, and will work correctly even when the constant is not present.

@ark99
Copy link
Contributor

ark99 commented Mar 16, 2016

Yes, that would work @davidalger

@davidalger
Copy link
Member

@unfunco Would you mind updating the PR to use one of the methods shown in my above comment?

@unfunco
Copy link
Member Author

unfunco commented Mar 16, 2016

I don't mind. I can update, but just before I do; it's worth considering that this would still evaluate successfully for versions 5.2.7 and lower, it would throw an E_NOTICE but the expected behaviour would still work, since the constant is undefined, it will assume the string "PHP_VERSION_ID" instead.

<?php
// Purposefully spelled incorrectly as a demonstration.
if (PHP_VERSON_ID < 50500) {
    echo 'I am less than version 5';
}

And I've only just seen your second example, @davidalger, which suppresses the E_NOTICE. Do we want suppression of errors in the condition or should this be the concern of the person defining the error reporting level?

@davidalger
Copy link
Member

@unfunco I would use the suppression as in my example. E_NOTICE is enabled in app/bootstrap.php which sets the error_reporting level to E_ALL.

@eddielau
Copy link
Contributor

@unfunco I think we should avoid using suppression. Would you mind changing it to use the first proposed method?

@unfunco
Copy link
Member Author

unfunco commented Mar 16, 2016

@eddielau I'd prefer to avoid suppression too, and in some basic testing the following:

<?php

if (!defined('PHP_VERSION_ID') || PHP_VERSION_ID < 50500) { ... }

performed faster than suppression and the version_compare that is currently used, it's both faster to fail and succeed since the second condition comparison is integer based.

I think the choice is with @davidalger (since he has Magento member next to his name) – I'm happy to change if required.

@davidalger
Copy link
Member

@unfunco Well I'd say just go with whichever tests out as faster, which sounds like that's using the defined() call to check presence. Also seems like the real Magento Team (i.e. @eddielau) would agree. I may have Magento Team next to my name, but ultimately the standards are up to the core team, as I'm just a moderator here on GitHub, so he would win even if I differed. ;)

@unfunco
Copy link
Member Author

unfunco commented Mar 16, 2016

Okay @davidalger, whatever @eddielau prefers I shall change it to, apologies @eddielau, GitHub didn't allow me to distinguish between member statuses.

@eddielau
Copy link
Contributor

Cool..Thank you!

@unfunco
Copy link
Member Author

unfunco commented Mar 16, 2016

I have updated the pull request, and rewrote the git history to modify the last commit.

@magento-team magento-team merged commit 687c540 into magento:develop Mar 25, 2016
magento-team pushed a commit that referenced this pull request Mar 25, 2016
- changed README text according to doc feedback
magento-team pushed a commit that referenced this pull request Mar 25, 2016
@unfunco unfunco deleted the optimisation/version-compare branch March 27, 2016 13:52
magento-team pushed a commit that referenced this pull request Nov 28, 2018

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
[Plankton] CHANGELOG.md for 2.2.7 Release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants