Path alias validation should test for relative path, no trailing slash requirements

I could not get the error messages to appear until I moved the patch in the path.admin.inc file. After that the error messages showed correctly in the UI.

I see what you are getting at with the taxonomy which your patch fixes and mine missed. However setting the admin form for a url alias allows both leading and trailing slashes as that form appears to run through the path_admin_form functions. I've rerolled with both sets of patches and this outputs the error messages to the UI for the user in both cases.

Hey all, following the issue here. I've added some testing for the admin form in addition to the current submitted testing improvements.

+++ b/core/modules/path/path.admin.inc
@@ -235,6 +235,15 @@ function path_admin_form_validate($form, &$form_state) {
+  if (drupal_substr($alias, 0, 1) == '/') {
+    form_set_error('alias', $form_state, t('The alias must be a relative path.'));
...
+  if (drupal_substr($alias, -1) == '/') {
+    form_set_error('alias', $form_state, t('The alias must not have a trailing slash.'));

+++ b/core/modules/path/path.module
@@ -143,6 +144,15 @@ function path_form_element_validate($element, &$form_state, $complete_form) {
+    if (drupal_substr($alias, 0, 1) == '/') {
+      form_error($element, $form_state, t('The alias must be a relative path.'));
...
+    if (drupal_substr($alias, -1) == '/') {
+      form_error($element, $form_state, t('The alias must not have a trailing slash.'));

We're trying not to introduce anymore calls to derepcated procedural wrappers; this should be changed to Unicode::substr(), \Drupal::formBuilder()>setError()/getError().

Looks good!

I don't mind committing this, since it's test coverage of an existing code path that previously lacked it, and that's +++.

However, from a usability POV, it would be *way* nicer if Drupal would just strip the trailing slash for you if you happened to provide it. It's like forms that ask you for a phone number and then yell at you when you have "-" in them. I could remove it, but a computer could also remove it just as easily.

Is there a reason not to just do that here, for better UX?

I'm going to take a stab at rerolling this since the directory structured changed. But also, I'm agreeing with @webchick, that it is annoying to have to reenter something that the validation could also fix, and this is a pretty simplistic replacing of unnecessary "/"

I redid the patch to trim leading and trailing slashed in aliases, to test that they are removed, and to reduce the complexity of the help text.

It should trims both "\" & "/"

Fixed function name and array index.

+++ b/core/modules/path/src/Form/PathFormBase.php
@@ -104,7 +104,7 @@ public function buildForm(array $form, FormStateInterface $form_state, $pid = NU
+      '#description' => $this->t('Specify an alternative path by which this data can be accessed. For example, type "about" when writing an about page. Use a relative path.'),

+++ b/core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php
@@ -58,7 +58,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+      '#description' => $this->t('The alternative URL for this content. Use a relative path. For example, enter "about" for the about page.'),

It is weird that in one place the description is about data and in the other it is about content (I realize that this is nothing new, but still might be worth unifying on this occasion). Also would it be possible to use exact the same sentence (less work for translators)?

One thing which is also not clear from the description is that its possible to use slashes in order to build up content hierarchies (reflected in breadcrumbs).

@TravisCarden: Do you think it is worth addressing #41 or are there any reasons against?

trim() can do the whole job in a single invocation. This trims all whitespace characters and both kinds of slashes.

 $alias = trim($alias, " \t\n\r\0\x0B\\/"); 

I feel this is much less readable than trim(trim($alias), " \\/");. Other than that the patch is ready.

I leave it to the person working on the patch and the committer on whether or not #41 should be addressed.

@TravisCarden: I did not realize this ramifications.

Note to committers: This was RTBC twice before (see #20 and #26). All of the concerns expressed there have been addressed.

There was a short discussion starting at #41 on whether or not to reword the description. #47 and #48 are about whether to use one trim or two of them (this is a question about readability vs catching edge cases).

Regardless I think this is ready.

This is probably not a problem, because linking to such things does not really make sense, but wanted to note that this will disallow linking to relative URIs such as //example.com/foo.js.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 79d21e7 and pushed to 8.0.x. Thanks!

Follow-Up to backport to Drupal 7: #2962374: [D7] Path alias validation should test for relative path, no trailing slash requirements