Make WordPress Core

Opened 12 months ago

Closed 8 months ago

Last modified 3 months ago

#59636 closed enhancement (wontfix)

Add meta to the revisions screen

Reported by: adamsilverstein's profile adamsilverstein Owned by: adamsilverstein's profile adamsilverstein
Milestone: Priority: normal
Severity: normal Version: 6.4
Component: Revisions Keywords: has-patch
Focuses: Cc:

Description

This ticket is a follow up to #20564 where we added opt-in support for meta revisions.

When meta is opted in to revisions, either at registration or using a filter, and stored as part of the revision storage, we should display that meta value on the revisions screen.

This core code would replace the filtered in display to the footer meta added by Gutenberg - WordPress/gutenberg@3e260aa/packages/block-library/src/footnotes/index.php#L92-L120.

Plugins can still overwrite the default display using the _wp_post_revision_field_{$meta_key} filter, the point of the change is that any revisioned value will be displayed by default.

This display is already covered by Gutenberg's "works with revisions" E2E test, this test should continue to pass even after this filter code is removed from Gutenberg. We can easily bring those tests over for core as well now that we use the same testing framework.

Attachments (1)

59636.diff (8.2 KB) - added by adamsilverstein 12 months ago.

Download all attachments as: .zip

Change History (14)

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


12 months ago
#1

  • Keywords has-patch added; needs-patch removed

Follow up to - follow up https://core.trac.wordpress.org/changeset/56745, this PR now includes the logic for displaying the revisioned meta on the revisions screen.

This core code would replace the filtered in display added by Gutenberg - https://github.com/WordPress/gutenberg/blob/3e260aa41d3c50684efac9728bdbba918658d131/packages/block-library/src/footnotes/index.php#L92-L120. Plugins can still overwrite the default display using the _wp_post_revision_field_{$meta_key} filter.

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

#2 @adamsilverstein
12 months ago

59636.diff includes changes from the PR.

#3 @jsmoriss
12 months ago

Note that if anything else than a string is returned by _wp_post_revision_field_{$meta_key}, then PHP issues an error:

PHP Warning:  trim() expects parameter 1 to be string, array given in wordpress/wp-includes/formatting.php on line 5506

Since metadata can contain other types than just strings (like arrays, for example), it would be nice if WP could handle those types returned by _wp_post_revision_field_{$meta_key}.

Here is an example of an array formatted using print_r() in the revisions page:

https://surniaulula.com/wp-content/uploads/screenshots/Screen%20Shot%202023-10-30%20at%204.34.00%20PM.png

And the code I used to hook those filters and format the metadata array:

https://github.com/surniaulula/wpsso/blob/master/lib/abstract/wp-meta.php#L250-L326

js.

Last edited 12 months ago by jsmoriss (previous) (diff)

@adamsilverstein commented on PR #5364:


11 months ago
#4

If you haven't already created a trac ticket for this improvement/enchancement specifically (I didn't find one on a quick search), I think it might make it easier to follow that way.

Yes good idea, I will go ahead and open up a follow up ticket to add this in 6.5

Here is the follow up ticket - https://core.trac.wordpress.org/ticket/59636

@adamsilverstein commented on PR #5364:


11 months ago
#5

If you haven't already created a trac ticket for this improvement/enchancement specifically (I didn't find one on a quick search), I think it might make it easier to follow that way.

Yes good idea, I will go ahead and open up a follow up ticket to add this in 6.5

Here is the follow up ticket - https://core.trac.wordpress.org/ticket/59636

#6 follow-up: @adamsilverstein
8 months ago

  • Milestone 6.5 deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

@jsmoriss

Was this already resolved elsewhere?

3 months ago
Note that if anything else than a string is returned by _wp_post_revision_field_{$meta_key}, then PHP issues an error:

Reconsidering whether we should add this automatically - developers would be better suited to know what they want to display on this screen, core should only handle fields we add directly (eg. footnotes). Closing as Wontfix, please re-open if I have missed something or this is still desired.

@adamsilverstein commented on PR #5364:


8 months ago
#7

@Mamaduka rethinking if we should add fields automatically, see my comment on the trac ticket. let me know what you think.

#8 in reply to: ↑ 6 @jsmoriss
8 months ago

Replying to adamsilverstein:

@jsmoriss

Was this already resolved elsewhere?

3 months ago
Note that if anything else than a string is returned by _wp_post_revision_field_{$meta_key}, then PHP issues an error:

Reconsidering whether we should add this automatically - developers would be better suited to know what they want to display on this screen, core should only handle fields we add directly (eg. footnotes). Closing as Wontfix, please re-open if I have missed something or this is still desired.

I don't think you understand the issue - plugin authors CANNOT add anything to this screen if the meta is not a string. A lot of plugin authors store values in arrays to optimize querying the post meta (one query vs many). If this isn't fixed, plugin authors cannot show the post meta differences if the post meta is an array.

As a concrete example, these two filters cannot be enabled:

https://github.com/surniaulula/wpsso/blob/main/lib/abstract/wp-meta.php#L176-L178

js.

@Mamaduka commented on PR #5364:


8 months ago
#9

Thank you, @adamsilverstein! Your reasoning makes sense to me.

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


4 months ago
#10

Trac ticket:

I can see that there was a similar closed ticket for this and a related pull request.

I just left a comment in the trac ticket to discuss how to proceed here.

@adamsilverstein @Mamaduka As you discussed this in the past, it would be great to hear your opinion on the best path forward.

## What?
Basically, this pull request shows the changes made to the post meta fields that have revisions enabled in the revisions UI.

## Why?
For 6.6, it is planned to allow the editing of custom fields when a block is connected to them. With that, I believe it is important to show the relevant changes in the revisions.

## How?
It iterates through the wp_post_revision_meta_keys, and it creates a new row in the diff for each of the meta fields that have changed.

I excluded the footnotes because it has its own section right now, but I am not sure if it should be unified in the proposed "Post Meta".

## Demo
This is a quick demo of how it works after the changes in this pull request:

https://github.com/WordPress/wordpress-develop/assets/34552881/7b41894d-8235-4943-98dd-6536eb3ead4a

#11 @santosguillamot
4 months ago

For 6.6, it is planned to allow the editing of custom fields when a block is connected to them: https://github.com/WordPress/gutenberg/issues/60956#issuecomment-2144790954.

With that, I believe it is important to show the relevant changes to post meta in the revisions. I wanted to bring this up again with a proposed solution, but I wasn't sure how to proceed. I'm happy to close this one and move the discussion to wherever it is considered.

@adamsilverstein @Mamaduka As you discussed this in the past, it would be great to hear your opinion on the best path forward.

Proposed solution: https://github.com/WordPress/wordpress-develop/pull/6735

@Mamaduka commented on PR #6735:


3 months ago
#12

Sorry for the late reply, @SantosGuillamot! This got lost somewhere in my notifications pile 😓

I think @adamsilverstein's previous decision still holds true. Developers registering post meta/block blindings are better positioned to decide what to display on the revisions screen.

Reconsidering whether we should add this automatically - developers would be better suited to know what they want to display on this screen, core should only handle fields we add directly (eg. footnotes).

@gziolo commented on PR #6735:


3 months ago
#13

Reconsidering whether we should add this automatically - developers would be better suited to know what they want to display on this screen, core should only handle fields we add directly (eg. footnotes).

Slightly related. That also nicely ties into the previous discussion on whether to share the feedback for users in the UI. In case of Footnotes it was treated as an internal change that never should be surfaced to the users, while for other custom post meta there might be sometimes temptation to highlight that to users.

Note: See TracTickets for help on using tickets.