Make WordPress Core

Opened 22 months ago

Closed 21 months ago

Last modified 21 months ago

#56271 closed defect (bug) (fixed)

Custom Template Parts are Duplicated, rather than being updated (PHP 5.6)

Reported by: jonmackintosh's profile jonmackintosh Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.0.2 Priority: normal
Severity: normal Version: 5.8
Component: Editor Keywords: has-patch has-unit-tests has-screenshots has-testing-info
Focuses: administration Cc:

Description

I actually logged this in the forums initially, https://wordpress.org/support/topic/site-template-issue-in-php-5-6/

Attempts to edit site template parts in WordPress 6.0.1 running on PHP 5.6.40 using the Twentytwentytwo theme result in duplicate template part being created as opposed to the actual template part being edited. This has a knock on impact. You can edit a template part but any subsequent attempt to edit the template part loses the previous changes, reverting the template part to its original state.

The issue cannot be recreated on PHP 7.4, and the functionality works as expected.

Image provided to illustrate PHP 5.6.40 outcome when attempting to change the background colour on the default header.

https://i.imgur.com/sHXrvAw.jpg

Attachments (1)

sHXrvAw.jpg (16.3 KB) - added by uofaberdeendarren 22 months ago.
Output on PHP 5.6.40

Download all attachments as: .zip

Change History (31)

@uofaberdeendarren
22 months ago

Output on PHP 5.6.40

#1 @uofaberdeendarren
22 months ago

  • Focuses administration performance added

#2 @uofaberdeendarren
22 months ago

The issue comes from this line here: https://github.com/WordPress/WordPress/blob/master/wp-includes/block-template-utils.php#L699

$query_result is an array of WP_Block_Template objects. The array_columns tries to "pluck" out the IDs for each of the template parts, to compare the current file-based template id to the database ones to see if the template already exists.

There is a difference in how array_columns works between PHP 5 and PHP 7. PHP 7 works fine when using objects, but PHP 5 does not. See: https://stackoverflow.com/a/23335938

That's why it works fine in PHP 7 because the return from array_columns does correctly contain the IDs of the templates, which will then stop the duplication. In PHP 5, the return is always an empty array, so the following conditionals don't think there are any database template parts, and therefore can duplicate them.

#3 @uofaberdeendarren
22 months ago

Working on a PR to resolve the issue.

This ticket was mentioned in PR #3010 on WordPress/wordpress-develop by darrencoutts118.


22 months ago
#4

  • Keywords has-patch added

An issue has been reported whereby template parts can be duplicated when running on PHP 5.6.

This has been tracked down to the use of array_column on an array of WP_Block_Template. Whilst this works correctly in PHP 7, it will always return an empty array in PHP 5.6. See: https://stackoverflow.com/a/23335938

I have converted the array_column call to array_map to ensure that this will continue to work with backwards compatibility.

Full details of the issue and the analysis can be found in the trac ticket.

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

#5 @uofaberdeendarren
22 months ago

  • Component changed from Themes to General
  • Summary changed from Site template issue in PHP 5.6 to Custom Template Parts are Duplicated, rather than being updated (PHP 5.6)

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


22 months ago

This ticket was mentioned in Slack in #core-php by uofa. View the logs.


22 months ago

#8 @rudlinkon
22 months ago

@uofaberdeendarren please update the PR with the Suggested changes which is reviewed by @costdev

#9 follow-up: @uofaberdeendarren
22 months ago

@rudlinkon changes are now made as requested.

#10 in reply to: ↑ 9 @rudlinkon
22 months ago

Replying to uofaberdeendarren:

@rudlinkon changes are now made as requested.

Thanks for your PR. It will be reviewed soon by @costdev

swissspidy commented on PR #3010:


22 months ago
#11

The change looks good to me. It would be great to add tests to tests/phpunit/tests/block-template-utils.php to prevent future regressions here.

#12 @swissspidy
22 months ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

darrencoutts118 commented on PR #3010:


22 months ago
#13

The change looks good to me. It would be great to add tests to tests/phpunit/tests/block-template-utils.php to prevent future regressions here.

Thanks, @swissspidy - Totally agree re unit tests, however, I suspect that @costdev (or indeed anyone else) would be quicker at adding them - I'm not too familiar with the codebase, let alone its tests. Happy to have a look, but I wouldn't count on me!

#14 @SergeyBiryukov
22 months ago

  • Component changed from General to Themes
  • Milestone changed from Future Release to 6.0.2

anton-vlasenko commented on PR #3010:


22 months ago
#15

I'm working on unit tests for this issue.

This ticket was mentioned in PR #3051 on WordPress/wordpress-develop by anton-vlasenko.


22 months ago
#16

  • Keywords has-unit-tests added; needs-unit-tests removed

This PR aims to fix a bug in the get_block_templates() method.
array_column doesn't work correctly with an array of objects on PHP <= 5.6. So, under certain circumstances, get_block_templates() could return entities with the same ID (slug).
A regression unit test (included in this PR) should prevent this from happening in the future.

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

anton-vlasenko commented on PR #3010:


22 months ago
#17

I've just submitted a new PR with the unit tests and the fix.
I propose to close this PR in favor of the new PR as it has unit tests.
Props to @darrencoutts118 for fixing the issue!

#18 @antonvlasenko
22 months ago

Steps to test https://github.com/WordPress/wordpress-develop/pull/3051:

  1. Make sure you are on PHP 5.6.x.
  2. Open the Editor and go to the list of templates/template parts.
  3. Modify any template/template part. Save it.
  4. Open the list of templates/template parts again.

Expected result:
The template/template part you've just edited should be displayed once.
Actual result:
The template/template part you've just edited will be displayed twice.

After the fix the template/template part will displayed once.

Last edited 22 months ago by antonvlasenko (previous) (diff)

anton-vlasenko commented on PR #3051:


22 months ago
#19

Thanks @anton-vlasenko! This review is targeted at formatting and organisation of methods.

Thanks for the review, @costdev!
I've replied to your comments.

darrencoutts118 commented on PR #3010:


22 months ago
#20

Thanks for putting the tests together @anton-vlasenko!

Slightly cheeky question... given I found the issues/came up with the original solution... will I still get credit on the contributor's page? 😄

First-time contributor, but would be nice to get my name there!

costdev commented on PR #3010:


22 months ago
#21

Thanks for putting the tests together @anton-vlasenko!

Slightly cheeky question... given I found the issues/came up with the original solution... will I still get credit on the contributor's page? 😄

First-time contributor, but would be nice to get my name there!

Not a cheeky question 🙂

As you submitted the original patch, and the Trac ticket includes this in one of the comments:

Props to @darrencoutts118 for fixing the issue!

So you'll be included in the credits when this is committed! See the Handbook entry on Commit Props (and the "Guidelines" section below it) for more information on how props are determined.

I'd suggest that you register an account on WordPress.org so that the link in the "Credits" page can link to your WordPress.org profile. You can also optionally add your GitHub username to your WordPress.org profile so others can find you here too 🙂

Thanks for contributing to WordPress Core! There is more information in the FAQ for New Contributors, should you need it! 🙂

anton-vlasenko commented on PR #3010:


22 months ago
#22

Thanks for putting the tests together @anton-vlasenko!

Slightly cheeky question... given I found the issues/came up with the original solution... will I still get credit on the contributor's page? 😄

First-time contributor, but would be nice to get my name there!

That's an entirely valid question, @darrencoutts118.
I'm not a release lead, so I can't tell for sure if you will get credit on the contributor's page. But I think you should get it.
I had to create a new PR only because I do not have write access to your repository.
I will modify the description section of my PR to make your contribution more obvious.

#23 follow-up: @ironprogrammer
22 months ago

  • Keywords has-screenshots has-testing-info added

Test Report

Patch tested: PR 3051 - LGTM 👍🏻

Environment

  • OS: macOS 12.5
  • Browser: Safari 15.6
  • Server: nginx/1.23.1
  • PHP: 5.6.40
  • WordPress: 6.0.2-alpha-53697-src
  • Theme: twentytwentytwo v1.2

Actual Results

  • ✅ After patching, customized Templates and Template Parts are no longer displayed in duplicate.

Additional Notes

  • Before patch is applied, custom templates are repeatedly added between lists when switching between Templates and Template Parts. See this video demonstration.
  • Also verified that after upgrading from PHP 5.6 to 7.4, templates were not shown in duplicate.

Supplemental Artifacts

Figures 1-2: Templates and Template Parts screens listing customized templates multiple times.

https://cldup.com/zwamIF5Qcg.png https://cldup.com/PjV6ct-qDg.png

Figures 3-4: Templates and Template Parts screens displaying non-matching template types between screens.

https://cldup.com/xr-3tYbKTK.png https://cldup.com/hMNNT27nOr.png

Figures 5-6: AFTER PATCH. Templates and Template Parts screens listing customized templates only once. ✅

https://cldup.com/MT7bctJDdM.png https://cldup.com/OTNwZ-odBV.png

#24 @ironprogrammer
22 months ago

  • Component changed from Themes to Editor
  • Focuses performance removed
  • Severity changed from major to normal
  • Version changed from 6.0 to 5.8

get_block_templates() was introduced in 5.8. As this is a display issue, removed performance focus and adjusted severity.

#25 in reply to: ↑ 23 @rudlinkon
22 months ago

Replying to ironprogrammer:

Test Report

Patch tested: PR 3051 - LGTM 👍🏻

Environment

  • OS: macOS 12.5
  • Browser: Safari 15.6
  • Server: nginx/1.23.1
  • PHP: 5.6.40
  • WordPress: 6.0.2-alpha-53697-src
  • Theme: twentytwentytwo v1.2

Actual Results

  • ✅ After patching, customized Templates and Template Parts are no longer displayed in duplicate.

Additional Notes

  • Before patch is applied, custom templates are repeatedly added between lists when switching between Templates and Template Parts. See this video demonstration.
  • Also verified that after upgrading from PHP 5.6 to 7.4, templates were not shown in duplicate.

Thank you so much for your detailed test report, so it may be resolved now.

#26 @SergeyBiryukov
21 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 53927:

Editor: Ensure get_block_templates() returns unique templates or template parts.

The function was using the array_column() PHP function on an array of objects, which works as expected on PHP 7 or later, but not on PHP 5.6.

This resulted in customized templates being listed multiple times on the Templates and Template Parts screens, and being repeatedly added between lists when switching between the screens.

The issue is now resolved by replacing array_column() with the wp_list_pluck() WordPress core function, which provides consistent behavior beetween PHP versions.

Reference: PHP 7.0 Upgrade Notes: array_column().

Props uofaberdeendarren, antonvlasenko, ironprogrammer, jonmackintosh, costdev, hellofromTonya, swissspidy, rudlinkon.
Fixes #56271.

#27 @SergeyBiryukov
21 months ago

In 53928:

Editor: Ensure get_block_templates() returns unique templates or template parts.

The function was using the array_column() PHP function on an array of objects, which works as expected on PHP 7 or later, but not on PHP 5.6.

This resulted in customized templates being listed multiple times on the Templates and Template Parts screens, and being repeatedly added between lists when switching between the screens.

The issue is now resolved by replacing array_column() with the wp_list_pluck() WordPress core function, which provides consistent behavior beetween PHP versions.

Reference: PHP 7.0 Upgrade Notes: array_column().

Props uofaberdeendarren, antonvlasenko, ironprogrammer, jonmackintosh, costdev, hellofromTonya, swissspidy, rudlinkon.
Merges [53927] to the 6.0 branch.
Fixes #56271.

SergeyBiryukov commented on PR #3010:


21 months ago
#28

Thanks for the PR! Merged in r53927, backported to the 6.0 branch in r53928.

SergeyBiryukov commented on PR #3051:


21 months ago
#29

Thanks for the PR! Merged in r53927, backported to the 6.0 branch in r53928.

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


21 months ago

Note: See TracTickets for help on using tickets.