Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
7 changed files
with
414 additions
and
29 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johannes Is this related to https://bugs.php.net/bug.php?id=76243?
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is only patching 7.3 version?
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@motecshine This was backported down to 7.1.19. It missed 7.2 RC by a day so it should be available in 7.1.19 + 7.2.8 + 7.3.0.
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johannes
When trying to build PHP 7.3 on FreeBSD 11.1 i end up with this error after the commit:
Did the code changes the build-requirements to for example MySQL 8.x? How can i fix it?
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@t-zuehlsdorff curious, does your build include the compilation of the hash extension?
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KalleZ
No, hash is disabled per default on FreeBSD. When enabling it building works fine. This makes hash no longer optional. Unsure if this is a bug or not :D
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, because the code was directly including the hashing functions but the code does not take into account that it may disabled and therefore the headers included is essentially empty.
I think that this entire block should have guards for whether or not the hash extension is available, much like we do other places or simply make mysqlnd have a dependency on the hash extension. Personally I prefer the first option here.
I would report it as a bug at the bugs website so we can easily track this issue in case @johannes or one of the other mysql guys doesn't get around to do it. I also think its something that should be resolved before PHP 7.3 is released (cc @cmb69).
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, a bug report should be filed β unfortunately, bug.php.net appears to be unavailable presently.
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion we should treat the hash extension like date and always enforce it (same also for openssl imo) having such security-related features should be a key feature users can rely upon in 2018.
As a work-around one might add a guard (maybe extracting the hashing into mysqlnd as fallback ... otherwise it's hard to predict for users what will work and leaves users in the cold when a distributor, for whatever reason, decides to build hash as shared extension ...)
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference: this issue has now been filed under https://bugs.php.net/76600 (thanks!)
IMO, that should be discussed on internals@ and might need an RFC.
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are welcome :)
Sadly i just noticed last week, that the chance was committed to 7.1 and 7.2. Now both versions are broken at FreeBSD.
Do you think that it will break more if i just patch this out? :D
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not compile anymore. This commit enforces the - normally optional - hash extension to be compiled, but in FreeBSD it is disabled by default. Also the hash extension is an separate module to install, so i can't just switch it to on (and violate the POLA rule).
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johannes You can't compile mysqlnd anymore without ext/hash.
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest reverting this change from 7.1 and 7.2 entirely. Considering that we already had to apply a number of followup fixes after this change was committed (I remember both some more build issues and segfaults), and the fact that this is more enhancement than bugfix, I'd say it does not belong on stable branches.
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also https://bugs.php.net/bug.php?id=76651, which is quite likely related to this change as well.
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be missing from NEWS and http://www.php.net/ChangeLog-7.php#7.2.8, is that on purpose?
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted these changes from 7.1+ via 03740ef. The merge to 7.3 was tricky because there were many other changes in the meantime. Hope nothing went wrong there.
We can reapply these changes once the issue from https://bugs.php.net/bug.php?id=76651 has been resolved (though I'd recommend to go for 7.3+ only).
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I posted an RFC to make the hash extension permanent in PHP which cannot be disabled:
https://wiki.php.net/rfc/permanent_hash_ext
If it passes, I suppose this commit could be re-applied
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like making the hash extension permanent passed. We should get it re-applied to php 7.1+
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hash extension is only required as of PHP 7.4, and this change has already been reapplied there with fixed openssl handling via 4f06e67.
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though I'd recommend to go for 7.3+ only - but then you will be able to use MySQL 8 only when on php 7.3 , correct? I think it should be supported on all currently supported versions.
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vladyslavstartsev The mysqlnd_caching_sha2_auth_plugin is still not even supported on PHP 7.3.
d6e81f0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any update on getting this supported in php 7.2+? We use mysql 8 for everything now.