Add validation to reject invalid characters (?&#) in patterns

I'll take a shot at this.

I've attached a patch file, which implements a basic pattern validation, against the mentioned characters (?#&).

The validation can easily be extended to check for further characters which are replaced in URLs anyways (e.g. !), but I can also add a second validation to check against those characters.

I've also added a assertion to PathautoUiTest::testPatternsValidation(), but currently only check for #, since I just wanted to test the basic functionality. I could also expand it to check against all invalid characters, but since this is my first contribution to Drupal I was unsure if that is needed/wanted.

I basically tried to mimic the behaviour of "token_element_validate", but was unsure if I should add the newly created "pathauto_pattern_validate" to "after_build" as well.

+++ b/pathauto.module
@@ -153,3 +154,29 @@ function pathauto_entity_base_field_info_alter(&$fields, EntityTypeInterface $en
+    $invalid_characters = ['#', '?', '&'];

I think the idea is to not hardcode those settings but get them from the characters that we replace.

You can get the list from \Drupal\pathauto\AliasCleaner::getPunctuationCharacters

I agree, that all the characters listed in \Drupal\pathauto\AliasCleaner::getPunctuationCharacters don't belong into an entity-alias and I would prefer a more generic list than the one I've hardcoded as well, but in my opinion the list in getPunctuationCharacters is too broad to apply them to the whole pattern, since it contains things like '/','.' and '-' which are quite common in URLs.

The following example is taken from a comment in AliasCleaner:cleanAlias

[token1]/[token2]-[token3]/[token4]

should we really disallow the hypen there?
I think for archives/dates this is a quite common use-case.

Additionally, right now the list from getPunctuationCharacters is currently only applied to the token-values and not the whole pattern (cleanTokenValues vs cleanAlias), accordingly validating against the whole list of characters that get replaced, would change the behaviour of the module quite substantially.

you are right, that list alone isn't enough. If you check how it is used in \Drupal\pathauto\AliasCleaner::cleanString, we only want those that are either set to REMOVE or REPLACE. So instead of the function, you could actually just get the list directly from the configuration, if it's not in there then we also don't do anything.

And of course, the default configuration is actually incomplete right now because the only one in there is hyphen it seems?

So yeah, maybe its easier to just hardcode the view that we really care about.

Lets see what @davereid things about this.

Lets do this. Committed.

I'll take a shot, at porting this to Drupal7

I've provided a backport to Drupal 7 for this issue.

Since there was already a validation function in pathauto.admin.inc (_pathauto_validate_numeric_element() ), I've based the implementation on that instead of mimicking the behaviour of 'token_element_validate' which I did for Drupal 8.
I'm not sure about the function naming, especially when to prefix it with an underscore, but copied that from _pathauto_validate_numeric_element

I've also added an assertion to PathautoFunctionalTestCase::testPatternsValidation() but again only checked for '#' and not all three characters.

I've used the php5.4 array short syntax, which resulted in the failure of the test on PHP 5.3, this patch fixes it.

This patch does exactly what it should do - its an exact backport from the d8 variant. I reviewed the code, and tested it on my local machine:

The test are fine too.

Ok, the function naming of @peschmae was consistent to the existing _pathauto_validate_numeric_element(), thats why i didn't mentioned that. I renamed that function to pathauto_pattern_validate() and added it (with 1 small modification) to #after_build. Now the form gets validated when the form is loaded.

This patch adds the validation to after_build in the D8 version as well.

Since the previous patch is already merged, I didn't provide an interdiff (and it would just contain the same change as the attached patch)