Opened 8 years ago
Closed 8 years ago
#39445 closed defect (bug) (fixed)
Add class_exists() check before defining the PasswordHash class
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 4.7.4 | Priority: | normal |
Severity: | normal | Version: | 4.7 |
Component: | Bootstrap/Load | Keywords: | has-patch commit fixed-major |
Focuses: | Cc: |
Description
I have a project that calls wp-load.php, but after already loading the PasswordHash class. (It authenticates the user before loading WP).
After the update from WP 4.6.1 to WP 4.7, this now causes a PHP fatal error, because now wp-settings.php unconditionally calls require().
The attached patch will not define the PasswordHash class if it already exists.
Attachments (4)
Change History (31)
#1
@
8 years ago
- Milestone changed from Awaiting Review to 4.7.1
We should probably just revert [38371] here honestly. I can't see any benefits in loading the class on every pageload when it's not needed.
Failing that, switching it back to a require_once()
.
I'm milestoning this for 4.7.x for review.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#3
follow-ups:
↓ 7
↓ 22
@
8 years ago
Alternative: Revert [38371] in trunk/4.8, switch to require_once()
in wp-settings.php
for 4.7 to avoid extra file churn in the minor, but also avoid this scenario.
The con of changing this back, and even more of delaying it until 4.8, is that if anyone who between 4.7.0 and $release who expects the class to exist will hit a fatal. I can't see a reason for most users to need it.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#5
@
8 years ago
- Milestone changed from 4.7.1 to 4.7.2
Per today's bug scrub in #core: we're going to punt this to 4.7.2 given that there's no agreement on approach. @aaroncampbell will check with @dd32 on this later today in case an agreement on approach is made quickly and this then lands for 4.7.1.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#7
in reply to:
↑ 3
@
8 years ago
Replying to dd32:
Alternative: Revert [38371] in trunk/4.8, switch to
require_once()
inwp-settings.php
for 4.7 to avoid extra file churn in the minor, but also avoid this scenario.
The con of changing this back, and even more of delaying it until 4.8, is that if anyone who between 4.7.0 and $release who expects the class to exist will hit a fatal. I can't see a reason for most users to need it.
I'm with @dd32 here (even though I understand the desire to clean things up like this). Unless @wonderboymusic has reasoning for this beyond "cleaner files", I'd say we should just put it back how it was. I get that it was required in a handful of places, but that's because it was only used in a handful of places.
#8
@
8 years ago
I don't think we should do anything to support this use case, because calling wp-load.php
in the wild means every single class in the codebase would require this same treatment. However, I understand the desire to revert to mitigate breakage. I don't see WordPress moving in the direction my original change intended, so the original change becomes mostly unnecessary.
This ticket was mentioned in Slack in #core by jnylen. View the logs.
8 years ago
#10
@
8 years ago
- Milestone changed from 4.7.3 to 4.7.4
It looks like there is mostly a consensus here to fix this by reverting [38371]. If someone wants to handle this for 4.7.3, ideally today, feel free to move back into the 4.7.3 milestone.
#11
@
8 years ago
- Keywords has-patch dev-feedback added
As per above discussion, I have reverted the [38371] in my patch. please let me know if any changes on this or if I am wrong anywhere.
#12
@
8 years ago
@ketuchetan there is still a require
line in wp-settings.php
that needs to be removed. Also, your patch removes a blank line in wp-includes/post-template.php
that was not introduced by [38371].
This ticket was mentioned in Slack in #core by swissspidy. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by swissspidy. View the logs.
8 years ago
This ticket was mentioned in Slack in #core by swissspidy. View the logs.
8 years ago
#19
@
8 years ago
- Owner set to swissspidy
- Status changed from new to assigned
- Version changed from trunk to 4.7
#20
@
8 years ago
39445-alternative.diff is an alternative patch for 4.7 to use require_once()
. As suggested earlier, we could do a full revert in 4.8 and this smaller change in the minor release.
Do not define the PasswordHash class if it already exists