Make WordPress Core

Opened 10 months ago

Closed 5 months ago

Last modified 5 months ago

#58298 closed defect (bug) (fixed)

Deprecated errors on /wp-admin/revision.php?revision=2895&gutenberg=true

Reported by: thomask's profile thomask Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.2
Component: Revisions Keywords: php82 has-patch
Focuses: Cc:

Description

with debug true and latest version i am recieving this deprecated error messages when i try to open post revisions

Deprecated: Creation of dynamic property Text_Diff_Engine_native::$ychanged is deprecated in /var/www/vhosts/powerbox.one/httpdocs/wp-includes/Text/Diff/Engine/native.php on line 39

Deprecated: Creation of dynamic property Text_Diff_Engine_native::$xchanged is deprecated in /var/www/vhosts/powerbox.one/httpdocs/wp-includes/Text/Diff/Engine/native.php on line 39

Deprecated: Creation of dynamic property Text_Diff_Engine_native::$yv is deprecated in /var/www/vhosts/powerbox.one/httpdocs/wp-includes/Text/Diff/Engine/native.php on line 40

Deprecated: Creation of dynamic property Text_Diff_Engine_native::$xv is deprecated in /var/www/vhosts/powerbox.one/httpdocs/wp-includes/Text/Diff/Engine/native.php on line 40

Deprecated: Creation of dynamic property Text_Diff_Engine_native::$yind is deprecated in /var/www/vhosts/powerbox.one/httpdocs/wp-includes/Text/Diff/Engine/native.php on line 41

Deprecated: Creation of dynamic property Text_Diff_Engine_native::$xind is deprecated in /var/www/vhosts/powerbox.one/httpdocs/wp-includes/Text/Diff/Engine/native.php on line 41

Deprecated: Creation of dynamic property Text_Diff_Engine_native::$ychanged is deprecated in /var/www/vhosts/powerbox.one/httpdocs/wp-includes/Text/Diff/Engine/native.php on line 39

Deprecated: Creation of dynamic property Text_Diff_Engine_native::$xchanged is deprecated in /var/www/vhosts/powerbox.one/httpdocs/wp-includes/Text/Diff/Engine/native.php on line 39

Attachments (1)

582981-Text_Diff_Engine_native-declare-all-properties.patch (937 bytes) - added by jrf 10 months ago.

Download all attachments as: .zip

Change History (13)

#1 @SergeyBiryukov
10 months ago

  • Keywords php82 added
  • Milestone changed from Awaiting Review to 6.3

#2 follow-up: @jrf
10 months ago

  • Keywords has-patch added

Thank you @thomask for reporting this.

The Text_Diff library is an external dependency, however, AFAIK it is no longer externally maintained.

I have done a code search within all the files in the wp-includes/Text subdirectory and the Text_Diff_Engine_native class looks to be the only one with undeclared properties.

I have attached a patch fixing the above reported notices, as well as some additional undeclared properties.

The fact that these were not discovered earlier highlights that there are no tests at all within the WP test suite which run the code in this class in any way.

As this dependency is no longer externally maintained, IMO we should open a follow-up ticket to, at the very least, add cursory tests which run the code to allow for discovering PHP deprecations/notices/warnings on recent PHP versions, though preferably a full test suite should be added.

I wonder if it may be prudent to create a repo to maintain this code as a package within the WP organisation ?

#3 in reply to: ↑ 2 ; follow-up: @SergeyBiryukov
10 months ago

Replying to jrf:

The Text_Diff library is an external dependency, however, AFAIK it is no longer externally maintained.

The pear/Text_Diff project on GitHub had a release in 2019 and appears to have recent commits, though is indeed no longer actively maintained, as it has been deprecated and superseded by Horde_Text_Diff.

We could probably consider switching to the newer package. On at least one occasion though, we encountered a bug in the upstream version, see comment:5:ticket:41526, that was patched using a different approach in [42028] / #41526. As far as I can tell, there were no further changes upstream after the commits in question, so the issue still exists there.

I have not yet checked whether a similar fix for pear/Text_Diff has the same issue.

I wonder if it may be prudent to create a repo to maintain this code as a package within the WP organisation ?

Would it be worth comparing the current code in core with the pear/Text_Diff version and checking if a switch would be possible? Otherwise, it looks like we may indeed have to continue maintaining this package for the time being, either in core or separately, to ensure compatibility with newer PHP versions.

#4 @SergeyBiryukov
10 months ago

  • Component changed from Editor to Revisions
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

#5 @SergeyBiryukov
10 months ago

In 55752:

Code Modernization: Explicitly declare all properties in Text_Diff_Engine_native.

Dynamic (non-explicitly declared) properties are deprecated as of PHP 8.2 and are expected to become a fatal error in PHP 9.0.

There are a number of ways to mitigate this:

  • If it is an accidental typo for a declared property: fix the typo.
  • For known properties: declare them on the class.
  • For unknown properties: add the magic __get(), __set(), et al. methods to the class or let the class extend stdClass which has highly optimized versions of these magic methods built in.
  • For unknown use of dynamic properties, the #[AllowDynamicProperties] attribute can be added to the class. The attribute will automatically be inherited by child classes.

In this case, the properties, as used in the class methods, fall in the “known property” category.

Reference: PHP RFC: Deprecate dynamic properties.

Follow-up to [53942], [53948], [53949], [53952], [53953], [53954], [53957], [54037].

Props jrf, thomask.
See #58298.

#6 in reply to: ↑ 3 @jrf
10 months ago

Replying to SergeyBiryukov:

Replying to jrf:

The Text_Diff library is an external dependency, however, AFAIK it is no longer externally maintained.

The pear/Text_Diff project on GitHub had a release in 2019 and appears to have recent commits, though is indeed no longer actively maintained, as it has been deprecated and superseded by Horde_Text_Diff.

We could probably consider switching to the newer package. On at least one occasion though, we encountered a bug in the upstream version, see comment:5:ticket:41526, that was patched using a different approach in [42028] / #41526. As far as I can tell, there were no further changes upstream after the commits in question, so the issue still exists there.

I have not yet checked whether a similar fix for pear/Text_Diff has the same issue.

I wonder if it may be prudent to create a repo to maintain this code as a package within the WP organisation ?

Would it be worth comparing the current code in core with the pear/Text_Diff version and checking if a switch would be possible?

Ow, nice find! I completely missed that.

Yes, I think it would be very good if we could take the following actions (probably in a separate ticket though):

  • Compare the current version of the package in WP Core with the upstream version of pear/Text_Diff.
  • Compare the current version of the package in WP Core with the upstream horde/Text_Diff package.

Based on the findings, discuss follow-up steps.

If we'd decide to upgrade to either the newer version of pear/Text_Diff or to horde/Text_Diff, I do still think adding at least _some_ tests to safeguard the integration would be a good thing. That way we can also safeguard that the upgrade does not cause any avoidable problems (at least for those situations which we are testing for).

#7 @oglekler
8 months ago

  • Milestone changed from 6.3 to 6.4

Due to lack of final decision and lack of activity lately, I am moving this ticket to the 6.4 milestone.

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


6 months ago

#9 @oglekler
6 months ago

  • Keywords changes-requested added

This ticket was discussed during a bug scrub

@SergeyBiryukov and @jrf can you please follow with this? I think ticket summary needs to be changed, and also there is a need for safeguard, because we just cannot make changes on third-party code without any concerns. So, clear scope will be good.

Add props to @mukesh27

#10 @oglekler
5 months ago

  • Keywords close added; changes-requested removed

@SergeyBiryukov and @jrf,
I am suggesting to close this ticket as fixed and move discussion about further path with this package into another ticket, because this is out of the scope for this one.

This open PR is addressing the issue we had fixed from our side: https://github.com/pear/Text_Diff/pull/8/files

#11 @hellofromTonya
5 months ago

  • Keywords close removed
  • Milestone changed from 6.4 to 6.3
  • Resolution set to fixed
  • Status changed from accepted to closed

Following up ...

I do think this ticket was fixed in the 6.3 cycle via [55752].

The remaining action items discussed likely are better addressed in a new ticket as @jrf suggests:

Action 1:

Yes, I think it would be very good if we could take the following actions (probably in a separate ticket though):

  • Compare the current version of the package in WP Core with the upstream version of pear/Text_Diff.
  • Compare the current version of the package in WP Core with the upstream horde/Text_Diff package.

Based on the findings, discuss follow-up steps.

Action 2:

If we'd decide to upgrade to either the newer version of pear/Text_Diff or to horde/Text_Diff, I do still think adding at least _some_ tests to safeguard the integration would be a good thing. That way we can also safeguard that the upgrade does not cause any avoidable problems (at least for those situations which we are testing for).

@SergeyBiryukov or @jrf would you mind opening a new ticket for the next action items?

I'll go ahead and close this ticket, marking it as fixed in 6.3.

#12 @jrf
5 months ago

I've created ticket #59580 for the follow-up/sync with upstream.

Note: See TracTickets for help on using tickets.