close

Make WordPress Core

Opened 20 months ago

Closed 5 months ago

#61675 closed defect (bug) (fixed)

wp-activate.php tries to set undefined property

Reported by: skithund's profile skithund Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.9 Priority: normal
Severity: normal Version:
Component: Networks and Sites Keywords: has-patch early
Focuses: multisite, php-compatibility Cc:

Description

wp-activate.php tries to set cache_enabled property, which doesn't exist on all object cache implementations, triggering Creation of dynamic property WP_Object_Cache::$cache_enabled is deprecated error on PHP 8.2.

<?php
if ( is_object( $wp_object_cache ) ) {
        $wp_object_cache->cache_enabled = false;
}
?>

Change History (21)

Image

This ticket was mentioned in PR #7048 on WordPress/wordpress-develop by @skithund.


20 months ago
#1

  • Keywords has-patch added

Check that cache_enabled property exists on object cache object.

Trac ticket: https://core.trac.wordpress.org/ticket/61675

#2 Image @SergeyBiryukov
20 months ago

  • Milestone changed from Awaiting Review to 6.7

#3 Image @jrf
20 months ago

  • Keywords needs-patch added; has-patch removed

@skithund Thank you for reporting this. This shouldn't be possible with the WP native object cache classes as the #[AllowDynamicProperties] attribute is set.

Having said this, as the property is explicitly used and set in WP Core, I think it would be prudent to make this property an explicitly declared one on the WP_Object_Cache class anyhow (visibility: public) with documentation stating that "overload" classes should declare that property.

As for existing "overload" classes, I believe patches would be needed to explicitly declare the property on those as well.

While the patch proposed protects against the deprecation notice, it does not work with magic methods in place (which the WP_Object_Cache class has), so as things are, it would effectively disable the cache in most cases, which would break the functionality, which I don't think is your intention.

Also see: https://3v4l.org/kLAYu

Image

This ticket was mentioned in PR #7132 on WordPress/wordpress-develop by @snehapatil02.


20 months ago
#4

  • Keywords has-patch added; needs-patch removed

### Changes Made

  • Added the cache_enabled property to the WP_Object_Cache class.

Trac ticket: https://core.trac.wordpress.org/ticket/61675

#5 Image @snehapatil02
20 months ago

  • Resolution set to invalid
  • Status changed from new to closed

@jrf I've updated the WP_Object_Cache class to explicitly declare the cache_enable property and documented it accordingly. Please review the PR.

Last edited 20 months ago by snehapatil02 (previous) (diff)

#6 Image @sabernhardt
20 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Image

@tillkruess commented on PR #7048:


17 months ago
#7

@spacedmonkey Can we get this merged?

#8 Image @desrosj
17 months ago

  • Milestone changed from 6.7 to 6.7.1

Unfortunately, this one missed the boat with 6.7 RC1 due out any moment. I'm going to punt this to 6.7.1 for consideration in the next minor release.

Image

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


16 months ago

#10 Image @desrosj
16 months ago

  • Milestone changed from 6.7.1 to 6.8

Because of a few bug reports opened since 6.7 was released, the Core team is evaluating the need for a short 6.7.1 cycle (possibly next week).

To help prepare for this scenario in case it's decided to move forward, I'm going to punt this one. This issue was not introduced in 6.7, so it now falls outside of the focus for 6.7.1 as currently defined.

Also, I'm choosing 6.8 instead of 6.7.2 because it seems this may be better suited for a major release in order to properly communicate and test.

Image

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


13 months ago

Image

@audrasjb commented on PR #7048:


13 months ago
#12

@spacedmonkey is this ticket/PR still on your plate for 6.8?

Image

@spacedmonkey commented on PR #7048:


13 months ago
#13

No it isn't sadly.

#14 Image @spacedmonkey
13 months ago

  • Keywords early added
  • Milestone changed from 6.8 to 6.9
  • Owner set to spacedmonkey
  • Status changed from reopened to assigned

Sadly I run out of time to look at this one in this release. I assigning to myself and punting to WP 6.9 and early.

#16 follow-ups: Image @spacedmonkey
6 months ago

  • Keywords commit added

I think the patch from @skithund looks good. Marking as ready to commit.

#17 in reply to: ↑ 16 Image @rollybueno
5 months ago

Reproduction Report

Description

This report validates whether the issue can be reproduced.

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.29
  • Server: nginx/1.29.1
  • Database: mysqli (Server: 8.4.6 / Client: mysqlnd 8.2.29)
  • Browser: Chrome 140.0.0.0
  • OS: Linux
  • Theme: Twenty Twenty-Five 1.3
  • MU Plugins: None activated
  • Plugins:
    • Redis Object Cache 2.6.3
    • Test Reports 1.2.0

Steps to reproduce

  1. Enable multisite & debugging
  2. Allow new registrations in Network Settings
  3. Register new site
  4. Check the /wp-activate.php?key=

Actual Results

  1. ❌ Could not reproduce the reported issue under this environment.
  2. No deprecation notice or undefined property warning appeared during site activation, as well as on the debug.log.

Additional Notes

  • Tested on Multisite with the Redis Object Cache plugin active.
  • The issue may only be reproducible with older or custom object-cache drop-ins that omit this property.

Supplemental Artifacts

https://i.imgur.com/0i0veQ8.png

#18 in reply to: ↑ 16 Image @rollybueno
5 months ago

Replying to spacedmonkey:

I think the patch from @skithund looks good. Marking as ready to commit.

@skithund PR on https://github.com/WordPress/wordpress-develop/pull/7048 is closed bdw, but I couldn't reproduce using 8.2.29

#19 in reply to: ↑ 16 Image @SergeyBiryukov
5 months ago

  • Keywords commit removed

Replying to spacedmonkey:

I think the patch from @skithund looks good. Marking as ready to commit.

It appears that PR 7048 would not work as expected, see comment:3:

While the patch proposed protects against the deprecation notice, it does not work with magic methods in place (which the WP_Object_Cache class has), so as things are, it would effectively disable the cache in most cases, which would break the functionality, which I don't think is your intention.

Also see: https://3v4l.org/kLAYu

Last edited 5 months ago by SergeyBiryukov (previous) (diff)

#20 Image @SergeyBiryukov
5 months ago

Some history here:

  • [3011] introduced the WP_Object_Cache::$cache_enabled property along with the class.
  • [6539] removed the property when persistent cache was removed from core.
  • mu:1298 introduced this check in both wp-activate.php and wpmu_create_blog().
  • mu:1319 then removed the check (“No need for this debugging code”), but only in wpmu_create_blog().

Older external object cache implementations, e.g. Memcached or APC, still declare the property, but it does not affect anything aside from being declared.

While the patch proposed protects against the deprecation notice, it does not work with magic methods in place (which the WP_Object_Cache class has), so as things are, it would effectively disable the cache in most cases, which would break the functionality, which I don't think is your intention.

Rereading this again, it appears that attempting to disable the cache for wp-activate.php and wpmu_create_blog() was indeed the original intention here, but since it was subsequently removed from the latter, it should be safe to remove it from the former too, as this is a remnant from the WPMU merge and has not been doing anything in practice for the last 17 years or so.

#21 Image @SergeyBiryukov
5 months ago

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

In 60914:

Networks and Sites: Remove obsolete code from wp-activate.php.

The WP_Object_Cache::$cache_enabled property does not exist in core as of WordPress 2.5, and has no meaningful usage in external object cache implementations.

This check was originally added for debugging purposes, and is now a remnant from the WPMU merge.

Follow-up to [6539], mu:1298, mu:1319, [12603].

Props skithund, jrf, snehapatil02, debarghyabanerjee, tillkruess, audrasjb, spacedmonkey, desrosj, rollybueno, SergeyBiryukov.
Fixes #61675.

Note: See TracTickets for help on using tickets.