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

Latest Posts: Fix selected category on existing blocks #21359

Merged
merged 6 commits into from
Apr 20, 2020

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented Apr 2, 2020

Description

Fixes #21357 — The selected category attribute in Latest Posts was changed to an array, but no upgrade path was added for existing blocks. This caused problems on the frontend and in the editor, so this PR attempts to fix those. For the frontend, add an array check before using the categories attribute, and handle the non-array case. For the editor, I've added a deprecation that should kick in if categories is a string, which will convert categories into the array-of-objects format it expects.

This migration ideally needs the whole category object, as right now it displays the selected category as an empty token — would appreciate suggestions on this.
Screen Shot 2020-04-02 at 12 32 46 PM

To test

  1. Add a Latest Posts block with GB 7.7.1 or below, then check out & build this branch
  2. View your post, it should show only that category of posts
  3. Switch to the editor, again it should only show that category of posts in the preview
  4. Click the block, and look at the selected categories — it should have a token (though empty now, see above)

Types of changes

Bug fix (non-breaking change which fixes an issue)

@ryelle ryelle added the [Block] Latest Posts Affects the Latest Posts Block label Apr 2, 2020
@ryelle ryelle self-assigned this Apr 2, 2020
@github-actions
Copy link

github-actions bot commented Apr 2, 2020

Size Change: +981 B (0%)

Total Size: 840 kB

Filename Size Change
build/block-library/editor-rtl.css 7.09 kB -22 B (0%)
build/block-library/editor.css 7.09 kB -24 B (0%)
build/block-library/index.js 112 kB +86 B (0%)
build/block-library/style-rtl.css 7.16 kB +31 B (0%)
build/block-library/style.css 7.17 kB +31 B (0%)
build/components/index.js 198 kB -1 B
build/components/style-rtl.css 16.7 kB +119 B (0%)
build/components/style.css 16.7 kB +118 B (0%)
build/edit-navigation/index.js 3.54 kB +435 B (12%) ⚠️
build/edit-navigation/style-rtl.css 475 B +196 B (41%) 🚨
build/edit-navigation/style.css 475 B +195 B (41%) 🚨
build/edit-post/index.js 27.7 kB -66 B (0%)
build/edit-site/index.js 10.4 kB -59 B (0%)
build/edit-widgets/index.js 7.47 kB -67 B (0%)
build/editor/index.js 43.3 kB +5 B (0%)
build/element/index.js 4.64 kB -2 B (0%)
build/url/index.js 4.02 kB +6 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.01 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.24 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 105 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.1 kB 0 B
build/data-controls/index.js 1.25 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/style-rtl.css 5.02 kB 0 B
build/edit-site/style.css 5.02 kB 0 B
build/edit-widgets/style-rtl.css 4.65 kB 0 B
build/edit-widgets/style.css 4.64 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/style-rtl.css 3.48 kB 0 B
build/editor/style.css 3.47 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.32 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.91 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.28 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 788 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@youknowriad youknowriad requested a review from draganescu April 8, 2020 14:59
@youknowriad
Copy link
Contributor

Do you know If this bug is present on WP 5.4 too? (whether to add the "backport" label or not)

@ryelle
Copy link
Contributor Author

ryelle commented Apr 8, 2020

@youknowriad No, it looks like 5.4 doesn't have the muli-select category feature, so it isn't an issue.

@draganescu
Copy link
Contributor

Hi @ryelle and thank you for fixing this. This PR keeps the category filter in place.

I tested and, aside from the empty label in the category list, following the steps you provided, I now see Notice: Undefined index: items in /var/www/html/wp-includes/rest-api.php on line 1262 on the frontend on the page that has the filtered LatestPosts block.

The editor also shows up the same error. I'm trying to track this down as well :) as it was reported by @bph in #21550

@mcsf mcsf force-pushed the fix/latest-posts-categories branch from f774fe4 to 9aa653e Compare April 15, 2020 11:35
@aduth
Copy link
Member

aduth commented Apr 15, 2020

This migration ideally needs the whole category object, as right now it displays the selected category as an empty token — would appreciate suggestions on this.

While I can see that it would make the UI easier to implement if the full objects were immediately available, I believe the category ID is enough to be able to get these details from the REST API. To me, the attribute data should include the minimal data necessary to be able to reconstruct the block. Or, at least, all the rest of these details can be considered redundant if the ID is known, and redundancy may lead to excessive storage and risk of desync. The second of these is more concerning since, for example, details about the category can change while the ID remains static (including the label).

From my understanding of #20781, if all that was needed to support multi-select was the storage of the category ID, then I think ideally the attribute would be an array of numbers. Generally I'd discourage the use of schemaless object attributes whenever possible, as the lack of a well-defined and enforced structure can be a source of bugs (gallery block comes to mind).

That all being said, the changes here do seem sensible given the current state of the block, and assuming that this has already shipped in a version of the plugin, this would still be necessary regardless if there's a future change in direction to how the attribute is stored.

To the bug itself of the empty label, given all of the above, my ideal implementation would be one where the suggestions passed to the token field are constructed based on entities fetched from the API corresponding to the ID of the category.

There appears to be some prior art of this in the Post Tags block:

const links = tags.map( ( tagId ) => {
const tag = getEntityRecord( 'taxonomy', 'post_tag', tagId );

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

This comment was for 16169. Sorry.

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

I tested this locally and with @ryelle 's and @mcsf 's updates this fixes both #21357 and #21550, the only remaining issue is the empty category label after migration.

@mcsf
Copy link
Contributor

mcsf commented Apr 17, 2020

the only remaining issue is the empty category label after migration

Let's address it separately.

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

This fixes two open issues, works as expected and it further removes some code duplication as well. Let's merge it.

@ryelle
Copy link
Contributor Author

ryelle commented Apr 20, 2020

@mcsf's changes are working great in testing, if the empty category label isn't necessary for this PR, is it ready to merge?

(edit: didn't see @draganescu's approval until I posted this 🙂 )

@ryelle ryelle merged commit 0c37ba8 into master Apr 20, 2020
@ryelle ryelle deleted the fix/latest-posts-categories branch April 20, 2020 18:37
@github-actions github-actions bot added this to the Gutenberg 8.0 milestone Apr 20, 2020
@ellatrix ellatrix mentioned this pull request Jun 16, 2020
12 tasks
@JJ-Coding
Copy link

Sad to see this wasn't added in the 8.6.0 release.
Is there any ETA for this fix to be released?

@mcsf
Copy link
Contributor

mcsf commented Jul 27, 2020

Sad to see this wasn't added in the 8.6.0 release.
Is there any ETA for this fix to be released?

This PR was merged in April, landing in Gutenberg 8.0.

@JJ-Coding
Copy link

Sad to see this wasn't added in the 8.6.0 release.
Is there any ETA for this fix to be released?

This PR was merged in April, landing in Gutenberg 8.0.

Oh, my bad then. I still have this issue on the latest Gutenberg release & WordPress release.
Or is this only going to be added once WordPress releases v5.5?

(Apologies if this isn't the right place for asking these questions - this seemed to be the only place where I could find anything about this bug.)

@draganescu
Copy link
Contributor

Howdy @JJ-Coding - this is the best place! If you install the Gutenberg plugin you should have the update ready. If you use the default Gutenberg shipped with WordPress 5.4 (which is Gutenberg 7.5) you will have to wait for WordPress 5.5 to get the update.

@mcsf
Copy link
Contributor

mcsf commented Jul 28, 2020

I re-tested on the latest Gutenberg version and found no issues with this. For the record, I tested with the following deprecated content:

<!-- wp:latest-posts {"categories":"6"} /-->

@JJ-Coding
Copy link

Howdy @JJ-Coding - this is the best place! If you install the Gutenberg plugin you should have the update ready. If you use the default Gutenberg shipped with WordPress 5.4 (which is Gutenberg 7.5) you will have to wait for WordPress 5.5 to get the update.

Hey @draganescu ! Thanks for confirming that I'm not posting in the wrong place - I'd rather not be that guy...

I've got the latest Gutenberg plugin installed (v8.6.0) but I'm still experiencing this problem it seems.

Just now I copied @mcsf 's code and pasted it into my page and then updated the page. Once the updating was finished I reloaded the editing page and the code that I had pasted in turned into:
<!-- wp:latest-posts /-->

So it seems to just remove the categories code completely when it updates.
Any idea what could cause this strange behaviour? (A plugin perhaps?)

Thanks for the quick responses by the way! I really appreciate the help.

@JJ-Coding
Copy link

Howdy @JJ-Coding - this is the best place! If you install the Gutenberg plugin you should have the update ready. If you use the default Gutenberg shipped with WordPress 5.4 (which is Gutenberg 7.5) you will have to wait for WordPress 5.5 to get the update.

Hey @draganescu ! Thanks for confirming that I'm not posting in the wrong place - I'd rather not be that guy...

I've got the latest Gutenberg plugin installed (v8.6.0) but I'm still experiencing this problem it seems.

Just now I copied @mcsf 's code and pasted it into my page and then updated the page. Once the updating was finished I reloaded the editing page and the code that I had pasted in turned into:
<!-- wp:latest-posts /-->

So it seems to just remove the categories code completely when it updates.
Any idea what could cause this strange behaviour? (A plugin perhaps?)

Thanks for the quick responses by the way! I really appreciate the help.

Alright - I've disabled all my plugins and added the code on a seperate test page. That seems to do the trick.
I'm just going to assume there's some misbehaving plugin which doesn't like categories on the latest post block.

Consider my question solved. Thanks again for the responses @draganescu & @mcsf !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Latest Posts Affects the Latest Posts Block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Latest Posts: Category restriction not working on existing blocks
6 participants