Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#39445 closed defect (bug) (fixed)

Add class_exists() check before defining the PasswordHash class

Reported by: davidanderson's profile DavidAnderson Owned by: swissspidy's profile 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 years ago.
Do not define the PasswordHash class if it already exists
39445.patch (2.5 KB) - added by ketuchetan 8 years ago.
reverting [38371]
39445.2.patch (3.0 KB) - added by ketuchetan 8 years ago.
Made the changes as per the @jnylen0 suggestions.
39445-alternative.diff (558 bytes) - added by swissspidy 8 years ago.

Download all attachments as: .zip

Change History (31)

@DavidAnderson
8 years ago

Do not define the PasswordHash class if it already exists

#1 @dd32
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: @dd32
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 @jbpaul17
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 @aaroncampbell
8 years 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
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 @jnylen0
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.

@ketuchetan
8 years ago

reverting [38371]

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

#13 @jnylen0
8 years ago

  • Keywords needs-refresh added; dev-feedback removed

@ketuchetan
8 years ago

Made the changes as per the @jnylen0 suggestions.

#14 @swissspidy
8 years ago

  • Keywords needs-refresh removed

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

#17 @swissspidy
8 years ago

  • Keywords dev-feedback added

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


8 years ago

#19 @swissspidy
8 years ago

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

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

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


8 years ago

#22 in reply to: ↑ 3 @ocean90
8 years 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
8 years ago

  • Keywords commit added; dev-feedback removed

#24 @swissspidy
8 years 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
8 years ago

  • Component changed from General to Bootstrap/Load

#26 @swissspidy
8 years ago

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

#27 @swissspidy
8 years 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.