Widget Editor: Fix allow adding same image twice

[thisissandip]

Description

Fixes #32901

Adding same image twice in the widgets editor only saves one block. This PR fixes that.

This was happening because while saving/updating the blocks in the widget editor, duplicate widget instances were filtered. Whereas when multiple blocks (cover block/image block) use the same image, the id attribute of the blocks becomes identical and only the first block was saved.

How has this been tested?

1. Go to widget editor.
2. Insert an image block with an image.
3. Insert a new image block with the same image.
4. Click save
5. Reload the page and both the blocks should be saved.

Types of changes

bug fix

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[adamziel]

@thisissandip I understand that removing filtering solves the "duplicate" block problem, but won't it break the duplicate widget case it was originally meant to address?

[adamziel]

I wanted to test this patch with function-based widgets and then I ran into an even larger issue: #32960

[thisissandip]

@adamziel Thank you for taking a look! 😄

but won't it break the duplicate widget case it was originally meant to address?

No, it won't. The filtering was blocking the blocks in the widget area with the same id but most of the blocks don't have the id attribute. Moreover, having 2 similar blocks within a widget area should be allowed.

[adamziel]

most of the blocks don't have the id attribute

IIRC the function-based blocks do and this code is there to prevent adding more than one instance of these (because WordPress doesn't support that).

Moreover, having 2 similar blocks within a widget area should be allowed.

Fully agreed! I'm only saying that this check is there for a reason and removing it will break something else. It would be cool to rethink it somehow to both prevent adding multiple instances of the same function-based widgets AND support multiple similar images at the same time. I'm not sure how to approach that off the top of my head though

[thisissandip]

IIRC the function-based blocks do and this code is there to prevent adding more than one instance of these (because WordPress doesn't support that).

@adamziel Okay! I'll try some other approach then!

[getdave]

TBH I'm not clear from the code what the purpose of this removing widgets which have duplicate ID attribute. There's not a lot of context in the code and the comments don't illuminate much beyond the fact that we're doing filtering.

Given that not all blocks have an id attribute I wonder what we're trying to achieve here?

Perhaps @noisysocks or @talldan can advise?

[adamziel]

@getdave IIRC we filter out the widgets with duplicate IDs to prevent adding more than one instance of a widget implemented using a function (e.g. marquee_greetings). WordPress doesn't support having more than one instance of these, if you try to save multiple marquee_greetings in different sidebars you will run into undefined behaviors. It isn't easy to prevent Gutenberg from adding one more instance of a block, hence the next best thing: this custom filtering logic.

[thisissandip]

@adamziel I have a question. If I am not wrong the function-based widgets can be added with legacy widget block right? So wrapping the filter with a block type check can solve the issue.

[getdave]

@adamziel I have a question. If I am not wrong the function-based widgets can be added with legacy widget block right? So wrapping the filter with a block type check can solve the issue.

How about #32951 (review) ?

[getdave]

// Remove all duplicate reference widget instances for legacy widgets.
// Why? We filter out the widgets with duplicate IDs to prevent adding more than one instance of a widget
// implemented using a function. WordPress doesn't support having more than one instance of these, if you try to
// save multiple instances of these in different sidebars you will run into undefined behaviors.
const usedReferenceWidgets = [];
const widgetsBlocks = post.blocks.filter( ( block ) => {

    const { id } = block.attributes;

    if ( block.name === 'core/legacy-widget' && block.attributes?.id ) {
        if ( usedReferenceWidgets.includes( id ) ) {
            return false;
        }
        usedReferenceWidgets.push( id );
    }
    return true;
} );

[getdave]

I gave this a spin and it seemed to work.

I'd say we'd need some confirmation that the widget id guard is still correct as even with Adam's comment I (personally) don't have the full context/confidence that I've done this correctly.

[getdave]

Screen.Capture.on.2021-06-24.at.17-12-27.mov

[thisissandip]

Yes, it works! I tested with file and cover block too! 🎉

[thisissandip]

@getdave Yes! This is exactly what I was thinking!

[thisissandip]

@getdave I have updated the code! Thanks for the snippet!

[talldan]

This makes sense to me, the id attribute of the legacy widget is fundamentally different from other block's id attributes, so shouldn't be confused.

Thanks so much for your contribution @thisissandip!

[adamziel]

This looks great, thank you @thisissandip and @getdave !