Make WordPress Core

Opened 2 years ago

Last modified 9 months ago

#54931 new defect (bug)

get_block_template filter not fired when template saved in posts

Reported by: ianmjones's profile ianmjones Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 5.9
Component: Editor Keywords: has-patch needs-testing
Focuses: Cc:

Description

The \get_block_template() function fires a 'get_block_template' filter at the end that allows for plugins to filter the content etc.

However, this only works if the template was retrieved from a theme's template file, if the template has been edited it is saved into the posts table.

When a template (part) is found in the posts table the function exits early with the found template but does not wrap the return in a call to the 'get_block_template'.

This means plugins and custom code do not get a chance to filter the template's content or properties when re-editing after a template has had an initial edit.

Attachments (1)

54391.patch (559 bytes) - added by ianmjones 2 years ago.
Simple patch.

Download all attachments as: .zip

Change History (14)

@ianmjones
2 years ago

Simple patch.

#1 @ianmjones
2 years ago

  • Keywords has-patch needs-testing added

#2 @ianmjones
2 years ago

  • Summary changed from get_block_template filter not fired when saved in posts to get_block_template filter not fired when template saved in posts

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


2 years ago

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


10 months ago
#4

This PR aims to add missing class properties to ensure WP_Text_Diff_Renderer_Table is PHP 8.2 compatible.

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

@mukesh27 commented on PR #5453:


10 months ago
#5

Thank you, @anton-vlasenko, for the PR. Could you also include a brief description indicating that we added these variables for backward compatibility (BC)? This way, we can prevent someone from raising a ticket in the future to remove these variables, claiming they are unused.

@antonvlasenko commented on PR #5453:


10 months ago
#6

Thank you, @anton-vlasenko, for the PR. Could you also include a brief description indicating that we added these variables for backward compatibility (BC)?

Thank you for checking this PR, @mukeshpanchal27!
I've added more detailed descriptions to the class properties in 73ebb738b7c1c30485472604051627cdc5d7a9b6 to clarify their purpose.

This way, we can prevent someone from raising a ticket in the future to remove these variables, claiming they are unused.

If I understand you correctly, you're referring to this comment. These class properties weren't just declared to maintain BC. They were introduced to ensure compatibility with PHP 8.2. What I meant is that these properties are not assigned any value to ensure BC. In my opinion, this doesn't need to be explained in the code. But that's my opinion.

@hellofromTonya commented on PR #5453:


10 months ago
#7

To follow-up on the discussion of these properties:

Anton is right - they are not being declared on the class for BC. Rather, this patch is necessary to explicitly declare the properties that are being used in Core.

Why? Prior to this patch, the properties were dynamic and thus were accessible only through the magic methods. r56354 modified the magic methods for all WP supported PHP versions (not just PHP 8.2+) to raise a deprecation for all dynamic properties.

Core itself though should not be raising this new deprecation as it's properties should all be declared.

@hellofromTonya commented on PR #5453:


10 months ago
#8

I first reviewed this patch thinking - yes, let's add the properties as declared. But then looking at r56354 I remember the changes implemented put all declared properties into the $compat_fields property which then is used in the magic methods.

I wondering .. should these 3 properties also be added to the $compat_fields array of compatible fields, rather than declaring them on the class? In this way, the current behavior is maintained, i.e. these 3 will still use the magic methods as they always have.

@anton-vlasenko @mukeshpanchal27 what do you think?

@antonvlasenko commented on PR #5453:


10 months ago
#9

@hellofromtonya I'm looking at https://core.trac.wordpress.org/ticket/56034 and, as far as I understand, the recommended way of dealing with known, named dynamic properties is to declare them on the class.
I'm referring to this part of the ticket (highlighted text):
https://i0.wp.com/github.com/WordPress/wordpress-develop/assets/43744263/bd16bdf4-20b6-403a-a4ce-cd6798ba94aa
However, I agree that there is less risk in adding these properties to the $compat_fields array than in declaring them on the class. I need to investigate potential BC risks.

@hellofromTonya commented on PR #5453:


10 months ago
#10

@anton-vlasenko you're right. That is the recommended approach. Thanks for reminding me!

I'm curious of the previous behavior, i.e. before r56354. Let's double check before moving forward with committing this change.

@antonvlasenko commented on PR #5453:


10 months ago
#11

Thank you, @hellofromtonya.
Before https://core.trac.wordpress.org/changeset/56354, these properties were dynamic and were never set in the Text_Diff_Renderer's constructor because __isset returned false. Now, the behavior remains the same. The only difference is that these properties are now declared, so the __isset() magic method is no longer called for them.
IMO, having them declared on the class is fine. But I'm open to hearing alternative opinions.

@hellofromTonya commented on PR #5453:


10 months ago
#12

Thanks @anton-vlasenko. Testing before and after for these Core specific dynamic properties:

  • Before 6.4: they always returned null and could not be changed / set https://3v4l.org/FrRJL
  • But this PR changes that behavior by allowing them to now be set, but ... that is the plan per proposal on how to deal with "known named dynamic properties".

Known, named, dynamic property | Declare the property on the (parent) class

So the PR as it is now aligns to that plan.

Note: See TracTickets for help on using tickets.