WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.23k stars 4.08k forks source link

`FormTokenField`: Issues with multiple identical strings #62533

Open Luehrsen opened 2 months ago

Luehrsen commented 2 months ago

Description

The FormTokenField component encounters issues when handling multiple tokens with identical string values. Specifically, when two or more items have the same name or title, the component fails to manage them correctly. This reliance on string matching without unique identifiers causes a React error and prevents selection of the duplicate items.

Expected Behavior:

Current Behavior:

P.S.: I am aware that the FormTokenField is currently being rewritten. However, I believe it is important to report this issue regardless.

Step-by-step reproduction instructions

  1. Create a post tag called 'Identical Name'.

    • Navigate to the "Tags" section in the WordPress admin dashboard.
    • Add a new tag with the name 'Identical Name'.
  2. Create a second post tag called 'Identical Name2'.

    • In the same "Tags" section, add another tag with the name 'Identical Name2'.
    • Note: Creation of a tag with an existing name (e.g., 'Identical Name') is blocked by WordPress.
  3. Edit the second post tag to be 'Identical Name'.

    • Find the tag 'Identical Name2' in the list of tags.
    • Edit the tag and change its name to 'Identical Name'.
    • Note: The check on existing names is not active on editing tags.
  4. Create a new post.

    • Navigate to "Posts" in the WordPress admin dashboard.
    • Click "Add New" to create a new post.
  5. Edit post tags.

    • In the block editor, locate the tags input field.
  6. Type 'Identical Name' in the FormTokenField.

    • Begin typing 'Identical Name' in the tags input field.
    • Observe the behavior of the FormTokenField as it attempts to handle the duplicate tags.

Screenshots, screen recording, code snippet

No response

Environment info

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

mirka commented 1 month ago

Thanks for the report!

It is true that somebody needs to be responsible for ensuring uniqueness, but ultimately I don't think it can/should be FormTokenField itself. The consumer knows best about how it wants to deal with potential duplicates, and even if we required unique identifiers, the consumer would still be responsible for ensuring identifier uniqueness.

The repro instructions you posted are in fact problematic at the application level and can be considered a bug though. Would it be fair to reclassify that as an app bug, rather than a problem with FormTokenField itself?

Luehrsen commented 1 month ago

Thanks for the discussion and insights on this issue.

I would like to clarify a few points regarding the responsibility of not relying on uniqueness in FormTokenField:

  1. Issue with FormTokenField: This is indeed an issue with FormTokenField. The control should inherently handle potential duplicates by not relying on the uniqueness of the tokens. Instead, it should at least provide options for utilizing unique identifiers.

  2. Repro Instructions: The repro instructions were intended to illustrate that we cannot and should not assume that the tokens, which this control operates upon, will always be unique. It’s crucial to design the control to handle such scenarios gracefully.

  3. String Matching: Relying on string matching for uniqueness is generally a code smell. This approach can lead to various issues, especially when unique identifiers are available and can be utilized effectively to manage tokens.

  4. Edge Cases Handling: The presence of an edge case and a code comment in getTermIdByTermValue discussing non-unique tokens indicates that this is a known issue. Addressing it at the control level would provide a more comprehensive solution, ensuring that all edge cases are handled consistently.

  5. Performance Considerations: Implementing token management within the control can be optimized for performance. This centralized approach allows for more efficient handling of tokens, as opposed to having multiple consumers implement their own potentially less efficient solutions.

  6. Aligning with HTML Select Tag: By adding a unique identifier, we move FormTokenField more in line with the HTML select tag, which traditionally and by design supports separate keys and values. This alignment ensures a consistent developer experience and leverages established design patterns.

Side Note: After re-reading the implementation of getTermIdByTermValue, I see that the string is lowercased. So a value of EXAMPLE and exAMple would be considered equal, and the latter would not be selectable. There might be more underlying issues, especially in languages with extended character sets.

Given these points, it is evident that addressing the issue within FormTokenField is not only beneficial but necessary to ensure a robust, maintainable, and user-friendly control.