WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 5 months ago

#39445 closed defect (bug) (fixed)

Add class_exists() check before defining the PasswordHash class

Reported by: DavidAnderson Owned by: swissspidy
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)

phppass.diff (747 bytes) - added by DavidAnderson 8 months ago.
Do not define the PasswordHash class if it already exists
39445.patch (2.5 KB) - added by ketuchetan 5 months ago.
reverting [38371]
39445.2.patch (3.0 KB) - added by ketuchetan 5 months ago.
Made the changes as per the @jnylen0 suggestions.
39445-alternative.diff (558 bytes) - added by swissspidy 5 months ago.

Download all attachments as: .zip

Change History (31)

@DavidAnderson
8 months ago

Do not define the PasswordHash class if it already exists

#1 @dd32
8 months 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 months ago

#3 follow-ups: @dd32
8 months 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 months ago

#5 @jbpaul17
8 months 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.


6 months ago

#7 in reply to: ↑ 3 @aaroncampbell
6 months ago

Replying to dd32:

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.

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 @wonderboymusic
6 months 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.


6 months ago

#10 @jnylen0
6 months 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.

@ketuchetan
5 months ago

reverting [38371]

#11 @ketuchetan
5 months 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 @jnylen0
5 months 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].

#13 @jnylen0
5 months ago

  • Keywords needs-refresh added; dev-feedback removed

@ketuchetan
5 months ago

Made the changes as per the @jnylen0 suggestions.

#14 @swissspidy
5 months ago

  • Keywords needs-refresh removed

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


5 months ago

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


5 months ago

#17 @swissspidy
5 months ago

  • Keywords dev-feedback added

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


5 months ago

#19 @swissspidy
5 months ago

  • Owner set to swissspidy
  • Status changed from new to assigned
  • Version changed from trunk to 4.7

#20 @swissspidy
5 months 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.

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


5 months ago

#22 in reply to: ↑ 3 @ocean90
5 months ago

Replying to dd32:

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.

+1 for this approach.

#23 @swissspidy
5 months ago

  • Keywords commit added; dev-feedback removed

#24 @swissspidy
5 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 40387:

Load: Only load PasswordHash class when needed.

This reverts [38371] which loaded class-phpass.php early in wp-settings.php and in turn caused backward compatibility problems.

Props DavidAnderson, ketuchetan.
Fixes #39445.

#25 @swissspidy
5 months ago

  • Component changed from General to Bootstrap/Load

#26 @swissspidy
5 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#27 @swissspidy
5 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 40389:

Bootstrap/Load: Only load PasswordHash class once.

require_once prevents errors when loading WordPress and the class already exists.

See [40387].
Fixes #39445.

Note: See TracTickets for help on using tickets.