Sever Nav Link item entity link on URL change if ID already exists #68143
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm going to add test coverage for this as future refactorings about making the URL dynamic would benefit from this. |
|
Size Change: +298 B (+0.02%) Total Size: 1.89 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in c968267. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/16197729260
|
a26cf16 to
25b1efb
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the navigation link block to only sever its built‐in entity link when the URL’s hostname or path truly change, while preserving the link for harmless query or hash modifications.
- Introduces
shouldSeverEntityLinkhelper to compare hostnames/paths and plain permalink IDs - Updates
updateAttributesto unsetidand mark links “custom” only when needed - Adds extensive unit tests covering sever/preservation scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/block-library/src/navigation-link/update-attributes.js | Implements URL comparison logic and refactors attribute assignment |
| packages/block-library/src/navigation-link/test/edit.js | Adds 20+ tests for link severing and preservation on URL updates |
Comments suppressed due to low confidence (1)
packages/block-library/src/navigation-link/update-attributes.js:106
- [nitpick] The destructured variable
newIDmixes casing (ID in uppercase). Renaming tonewIdwould align with common camelCase conventions and improve readability.
id: newID,
packages/block-library/src/navigation-link/update-attributes.js
Outdated
Show resolved
Hide resolved
MaggieCabrera
approved these changes
Jul 14, 2025
c968267 to
a30b878
Compare
|
I don't know what is going on with these PHP tests 😕 |
I think it might just need a rebase after #70718 fixed up the Docker issue? |
- Add comprehensive E2E tests for navigation link block behavior - Test entity ID removal when URL is manually changed - Test entity ID retention when URL is changed via entity selection - Test custom URL handling without entity ID - Use programmatic approach with requestUtils.createNavigationMenu() - Follow established patterns from navigation.spec.js - Fix wp-env port configuration to avoid conflicts - Update global setup to handle missing test plugin gracefully
…L is manually changed - Add logic to update kind and type attributes to 'custom' when URL is changed without new entity ID - Add comprehensive unit tests to verify the new behavior - Update existing test expectations to match new behavior - Ensures navigation links properly indicate they are custom links after manual URL changes
…rmalinks and query/hash changes - Only sever entity link if the post ID in plain permalinks (?p=, ?page_id=) changes, or if switching between them. - Preserve entity link for changes to query string or hash that do not affect the entity. - Add and update tests for all relevant scenarios.\n\nFix lint errors.
- Accept both 'title' and 'label' from updatedValue for backward compatibility - Fix issue where label changes were not being processed correctly - Ensure all existing tests continue to pass
… entity link on harmless query/hash changes - Fixes bug where adding a query string to a URL with a hash fragment would sever the entity link due to inconsistent trailing slash handling. - Adds a unit test for this scenario. - Ensures /foo and /foo/ are treated as equal for entity link preservation.
- Update shouldSeverEntityLink to use base URL for relative URL comparison - Preserve entity links when changing to matching relative URLs (e.g., https://example.com/hello-world/ -> /hello-world/) - Sever entity links when changing to non-matching relative URLs - Use window.location.origin as base in browser, https://wordpress.org fallback for tests - Add tests for relative URL handling scenarios
a30b878 to
01c8eb3
Compare
|
@fabiankaegy as the Issue author, would you like to take a look at this PR before it is merged? 🙏 |
|
@getdave This is cool! Thanks for working on it and it does solve the problem I was running into :) |
USERSATOSHI
pushed a commit
to USERSATOSHI/gutenberg
that referenced
this pull request
Jul 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Fixes #67643: Navigation Link block keeps reference of post ID it was created with after URL gets changed.
This PR addresses the issue where navigation link blocks retain their original post/page ID even after the URL is manually changed by the user. The solution intelligently determines when to sever entity links based on the nature of URL changes, preserving entity links for harmless modifications like query strings and hash fragments.
Key Changes
Core Fix: Enhanced
updateAttributesfunction inpackages/block-library/src/navigation-link/update-attributes.jswith sophisticated URL comparison logic:shouldSeverEntityLinkhelper function that compares URL hostname and path to determine when entity links should be severed?p=and?page_id=parameters)kindandtypeattributes are updated to "custom"titleandlabelproperties from link suggestionsComprehensive Testing: Added 20+ unit tests covering various URL modification scenarios including pretty permalinks, plain permalinks, query strings, hash fragments, and protocol changes.
Why
The original issue caused navigation items to disappear from the frontend when:
This created a confusing user experience where navigation items would unexpectedly disappear even though the user had intentionally changed the URLs to point to valid destinations.
Additionally, the previous implementation was too aggressive - it would sever entity links even for harmless URL modifications like adding query strings or hash fragments, which should preserve the entity relationship.
How
The solution works by detecting when a user manually changes a URL and intelligently determining whether the change warrants severing the entity link. The
shouldSeverEntityLinkfunction compares hostname and path to detect actual entity changes, handles WordPress plain permalinks specially, and preserves entity links for query string and hash fragment modifications.This approach:
Testing
Unit Tests
Comprehensive test coverage for URL modification scenarios:
Entity Link Severing Tests:
Entity Link Preservation Tests:
Manual Testing Steps
Create a navigation link to an existing page/post
Test entity link severing
Test entity link preservation
?utm_source=test) or hash fragments (e.g.,#section)Test plain permalinks
?p=and?page_id=parametersDelete the original entity
Edge Cases to Test
Follow ups
We will need to apply the same fix to Submenus possibly by consolidating the implementation between these two and sharing across the blocks.
Breaking Changes
None - this is a bug fix that maintains backward compatibility.
Uncertain Elements
Related Issues