Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Footnotes: add missing _ in revision field filter #53135

Merged
merged 3 commits into from
Aug 1, 2023

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jul 29, 2023

What?

Props @adamsilverstein for bringing this to my attentions. I forgot to prefix a filter with _.

But for some reason the filter is working fine without the _! I'm not sure why? I turned off both this filter and the one in core, and footnotes are rendering fine in the revision diff.

https://github.com/WordPress/wordpress-develop/blob/78e9478de7ee5dc67975a6694ffd1657cf1843aa/src/wp-admin/includes/revision.php#L70-L93

I've also prefixed the function that have been named in #52870.

Why?

How?

Testing Instructions

Create a post with footnotes. Then update the content and update a footnote or add/remove one. Check the footnotes field on the revisions page.

Testing Instructions for Keyboard

Screenshots or screencast

@ellatrix ellatrix added [Type] Bug An existing feature does not function as intended [Type] Code Quality Issues or PRs that relate to code quality Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta [Block] Footnotes Affects the Footnotes Block labels Jul 29, 2023
@ellatrix ellatrix requested a review from ajitbohra as a code owner July 29, 2023 10:58
@ellatrix ellatrix removed the [Type] Bug An existing feature does not function as intended label Jul 29, 2023
@ellatrix ellatrix force-pushed the fix/fn-revision-field-filter branch 2 times, most recently from 91d6601 to 2753c11 Compare July 29, 2023 11:23
@github-actions
Copy link

github-actions bot commented Jul 29, 2023

Flaky tests detected in 6057cbe.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5721900070
📝 Reported issues:

return get_metadata( 'post', $revision->ID, $field, true );
}
add_filter( 'wp_post_revision_field_footnotes', 'wp_get_footnotes_from_revision', 10, 3 );
add_filter( 'wp_post_revision_field_footnotes', '_wp_get_footnotes_from_revision', 10, 3 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this hook name is incorrect here and needs a _ prefix, not the callbacks.

See: https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-admin/includes/revision.php#L90-L93

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look like I reverted a part of the earlier PR by accident.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added it back. Still doesn't explain to me why it was working fine previously.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, either.

Do you mind reverting callback renames? Those are unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Are they completely unnecessary? We don't want people to be using these functions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually underscore prefix indicates private functions, but there's no such thing as private global functions in PHP. If people want, they'll still use it.

Using closures was a good idea, but I don't fully understand why it was requested to revert that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, _ doesn't prevent someone from using it, but at least it communicates that it's not supposed to be used

@tellthemachines tellthemachines enabled auto-merge (squash) August 1, 2023 03:04
@tellthemachines tellthemachines merged commit 561d244 into trunk Aug 1, 2023
@tellthemachines tellthemachines deleted the fix/fn-revision-field-filter branch August 1, 2023 03:17
@github-actions github-actions bot added this to the Gutenberg 16.4 milestone Aug 1, 2023
tellthemachines pushed a commit that referenced this pull request Aug 1, 2023
* Footnotes: add missing _ in revision field filter

* Use correct hook name

* Revert prefixing callback names
@tellthemachines
Copy link
Contributor

I just cherry-picked this PR to the update/second-round-6-3-rc3 branch to get it included in the next release: 2d8d1da

@tellthemachines tellthemachines removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Aug 1, 2023
tellthemachines added a commit that referenced this pull request Aug 1, 2023
* Top toolbar: Fix issues with save button overlap, and plugin buttons (#53101)

* Shorten the width of the top toolbar overlay and make doc title smaller.

* Add a scrim and a margin to handle plugin buttons better.

* Remove block tools back compat component schedule for deprecated in 6.3 (#53115)

* Removes usage of BlockToolsBackCompat

* Remove unwanted BlockTools from Nav sidebar

* Footnotes/RichText: fix getRichTextValues for deeply nested blocks (#53034)

* Defer to preceding handlers in command palette keyboard shortcut (#53001)

* Image block: fix image size at wide and full width (#53184)

* Fix regression with Edit site Navigate regions (#52940)

* Make the navigabel region wrap the inert sidebar.

* Adjust animation.

* Fix not expanding pattern in page editor (#53169)


---------

Co-authored-by: Aaron Robertshaw <[email protected]>

* Footnotes: fix published preview (#53072)

* Footnotes: fix published preview

* remove var dump

* Fix php lint

* PHP lint

* Address feedback

* Add e2e test

* Footnotes: disable for synced patterns and prevent duplication for pages in site editor (#53003)

* Initial commit:
- Prevent footnote creation withing core/block
- Only insert a footnote if one isn't found in the entity block list

* Try grabbing controlled blocks from parent post content block

* Cache `selectedClientId`
Get hasParentCoreBlocks using separate useSelect call.

* Rename hasParentCoreBlocks to isBlockWithinPattern
Add comments

* Removing while loop since we're already fetching the post content parent in the `getBlockParentsByBlockName` call above

* Reinstating while loop because it can deal with nested blocks

---------

Co-authored-by: Andrew Serong <[email protected]>

* Footnotes: add missing _ in revision field filter (#53135)

* Footnotes: add missing _ in revision field filter

* Use correct hook name

* Revert prefixing callback names

* don't display BlockContextualToolbar at all in contentonly locking (#53110)

* Render the footer conditionally in the global styles sidebar component so that any side effects from the footer wrapper are not rendered, e.g., styles and what not (#53204)

Ensure that the precise bottom margin persists if the footer isn't rendered

* Pattern: Add getBlockRootClientId call (#53206)

---------

Co-authored-by: Joen A <[email protected]>
Co-authored-by: Dave Smith <[email protected]>
Co-authored-by: Ella <[email protected]>
Co-authored-by: Mitchell Austin <[email protected]>
Co-authored-by: Aki Hamano <[email protected]>
Co-authored-by: Andrea Fercia <[email protected]>
Co-authored-by: Kai Hao <[email protected]>
Co-authored-by: Aaron Robertshaw <[email protected]>
Co-authored-by: Ramon <[email protected]>
Co-authored-by: Andrew Serong <[email protected]>
Co-authored-by: Andrei Draganescu <[email protected]>
@adamsilverstein
Copy link
Member

But for some reason the filter is working fine without the _! I'm not sure why? I turned off both this filter and the one in core, and footnotes are rendering fine in the revision diff.

I think it uses the raw meta by default

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Footnotes Affects the Footnotes Block [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants