Improve URI character limit handling - #16977
Conversation
Add new sys setting for uri character limit and clear error messaging for invalid uri lengths. Also, consolidates alias prep method into shared Trait method to remove duplication in create & update processors.
Clarify 2 condition expressions in getAliasPath method
Formatting only
Code quality fixes only
Remove maxLength from uri component
Add basic tests for alias/uri creation to ensure new UrlElementsTrait maintains functionality of the two methods it replaces. Additionally very basic test of newly introduced uri length limits.
Fix - need FURLs on for some new tests
A Different (Almost) Solution Set Aside for NowJust for reference, I'd initially tried to solve this with emitting the URI field error in mind, but it became too difficult to properly maintain the correct state of the alias and uri along the way — it can be done, but I concluded it wasn't worth the effort right now (I'd argue a general refactor of how the whole alias/uri generation works and how it's placed in the UI will be in order for some future MODX version). In this version, the URI is always visible, but disabled unless Freeze URI is on OR there was an error on the field. Also, a live character counter is shown on the errored field once editing begins. Here's what it was looking like: modx-uri-error-solution-1.mov |
|
Lil' bit worried about the modx.js change making it render HTML. That might trigger for a bunch of different scenarios, including when rendering HTML is not expected. Perhaps at the very least parse that through the safeHtml utility we added a few versions ago? |
Although I don't anticipate this ever seeing user input (at least that which isn't controlled by the core or Extras), it's not a bad idea to sanitize the message at that point to be sure (although if a message makes use of user input it probably should have been sanitized beforehand). I'll add that in this evening. Just in case, one thing to be clear on is this change only allows html to be used in the error messaging, assuming it was properly encoded on the php end of things; it doesn't expect html or transform the incoming content in any way other than decoding it. Good to see you back in action! |
Apply safeHtml to showError msg
|
UPDATE: Applied The completely ideal implementation would be to have a new modx.js method $this->failure('A message', ['isFormatted' => true]);But, that array data is nowhere to be found in the js failure response. |
What changed and why
Removed hard front end limit on URI field and added limits based on the
cache_alias_mapsystem setting:uri_max_lengthsettingWhen these limits are reached, a general error is emitted instead of trying to show a field error on the URI input. This choice was made because it's difficult to reconcile the fact that this field's derived value can be invalid whether or not Freeze URI is on—but it needs to be on to see an error when it's emitted on the field itself.
*Manual testing shows that when uris go beyond 191 characters, particularly when the first 191+ characters are not unique (imagine documents within a very deeply nested folder with relatively long alias lengths along the way), individual documents at that depth cannot be navigated to reliably (or at all).
Error Display
The new error is relatively verbose about how to resolve the issue and provides direct links to the system settings that can be changed to overcome the limit causing the error. This brief video shows what to expect:
modx-uri-error-solution-2.mov
HTML in Error
One other small change to the js allows the general error to include html by changing the message method from
alerttoshow. This will not affect current implementations but will allow html messages when they're properly json encoded. (See the change tomodx.js.)How to test
grunt buildis requiredcache_alias_mapis offcache_alias_mapon, change theuri_max_lengthsetting to a reasonable length to test its operation; I used a short length of 80 (and even less) to easily verifyRelated issue(s)/PR(s)
Alternate to PR #16895; solves #13815
Although this PR covers an arguably edge-case situation, it adds the benefit of allowing admins to limit URIs to they're liking to keep content editors from running wild with complicated/long structures and titles.
Compatibility notes
n/a
Breaking change assessment
None. Temporary BC references from old methods (marked deprecated) to new Trait method.
Test coverage
New cases added to ResourceCreateProcessorTest for basic coverage for new Trait takes the place of
Create->prepareAliasandUpdate->checkFriendlyAliasand URI character length limitsContributors
Review of @Ibochkarev's initial proposed solution prompted this alternate one.
AI tool use
n/a