Clean up Drupal\jsonapi\Access\CustomParameterNames and make it follow the spec more closely

Cool :)

I'll be pushing this forward later today.

Straight reroll. This didn't apply anymore, especially due to #2844717: Validation of parameter names doesn't correctly check for backslash and control characters :)

Updating the IS to reflect what is already done.

#2844717: Validation of parameter names doesn't correctly check for backslash and control characters did a great job of also testing against the ASCII control characters, which is mentioned at http://jsonapi.org/format/ as:

U+0000 to U+001F (C0 Controls)

But really, DEL (0x7F or U+007F in Unicode notation) should also be tested explicitly, since it's usually considered part of the C0 controls: https://en.wikipedia.org/wiki/C0_and_C1_control_codes#C0_.28ASCII_and_de...

While not technically part of the C0 control character range, the following two characters are defined in ISO/IEC 2022 as always being available regardless of which sets of control characters and graphics characters have been registered. They can be thought of as having some characteristics of control characters.

• space (U+0020)
• DEL (U+007F)

The JSON API spec specifically calls out that space is allowed but not recommended (because it's not URL-safe).

This resolves one of the @todos I had added in #2.

Thanks for the review & reroll, @dawehner!

A big update will follow shortly.

The code is now called CustomParameterNamesAccessCheck, but it's wrong on at least two levels:

1. much of the logic is the same for member names, not just parameters and custom parameters
2. actually, this is validating parameters (as in official ones), not custom parameters — this is what the spec writes about custom parameters:
   

   Implementation specific query parameters MUST adhere to the same constraints as member names with the additional requirement that they MUST contain at least one non a-z character (U+0061 to U+007A). It is RECOMMENDED that a U+002D HYPHEN-MINUS, “-“, U+005F LOW LINE, “_”, or capital letter is used (e.g. camelCasing).

   If a server encounters a query parameter that does not follow the naming conventions above, and the server does not know how to process it as a query parameter from this specification, it MUST return 400 Bad Request.

   Note: This is to preserve the ability of JSON API to make additive additions to standard query parameters without conflicting with existing implementations.

Conclusions:

1. we should move the logic for validating member names out of this class, and just reuse it
2. we should be distinguishing (official) parameter names vs custom/implementation-specific parameter names

The fact that CustomParameterNamesAccessCheck is muddying all these concepts together makes it A) hard to understand, B) not future-proof.

So here's a first step: moving the "restricted characters" to a "JSON API spec" class.

I noticed that the existing test coverage in \Drupal\Tests\jsonapi\Unit\Access\CustomParameterNamesAccessCheckTest is incomplete in at least two important ways:

1. Member names MUST contain at least one character. is not tested: we should test that a single-character member name/parameter name is considered valid
2. U+0080 and above (non-ASCII Unicode characters; not recommended, not URL safe) is not tested: we should test that the "highest" allowed character is indeed allowed: U+10FFFF

In adding those two test cases, I also noticed that the "unsafe chars" test cases were using ” instead of " and ’ instead of '. So, fixed that too. This should now have several failing tests.

As predicted, #16 failed. It has these 3 failures:

1) Drupal\Tests\jsonapi\Unit\Access\CustomParameterNamesAccessCheckTest::testJsonApiParamsValidation with data set "unicode-above-u+0080-highest-allowed" ('12\u{10FFFF}', true)
Failed asserting that false is true.

/var/www/html/modules/contrib/jsonapi/tests/src/Unit/Access/CustomParameterNamesAccessCheckTest.php:29

2) Drupal\Tests\jsonapi\Unit\Access\CustomParameterNamesAccessCheckTest::testJsonApiParamsValidation with data set "unsafe-"" ('kitt"ens', false)
Failed asserting that true is false.

/var/www/html/modules/contrib/jsonapi/tests/src/Unit/Access/CustomParameterNamesAccessCheckTest.php:32

3) Drupal\Tests\jsonapi\Unit\Access\CustomParameterNamesAccessCheckTest::testJsonApiParamsValidation with data set "unsafe-'" ('kitt'ens', false)
Failed asserting that true is false.

So the first new test case in #16 actually passed. The second did not, so the current validation is incorrect.

Finally, the incorrect "unsafe chars" I noticed that I fixed also resulted in failures. This means that that validation is incorrect as well.

This fixes all that.

At this point, because of the improved (now actually correct) validation of member names, it becomes pointless/unnecessary to validate against the list of restricted characters too. The restricted characters are specifically called out for less strict implementations. But given that we can now strictly validate against the list of allowed characters (as defined by the JSON API spec), we can simplify things a lot.

This moves the bulk of the tests over from CustomParameterNamesAccessCheckTest to JsonApiSpecTest. This allows for proper unit testing.

This addresses #22.3. It was already in progress before #22.3 was posted :)

This adds a \Drupal\jsonapi\JsonApiSpec::isValidCustomQueryParameter method, as per #15.

Includes additional unit test coverage to ensure the additional requirements are met.

This also means the final remaining @todo can be removed :)

+++ b/tests/src/Unit/JsonApiSpecTest.php
@@ -0,0 +1,112 @@
+    $data['unicode-above-u+0080-highest-allowed'] = ["12\u{10FFFF}", TRUE];

This is what is causing the test failures on PHP 5. Apparently it's a PHP 7 feature: http://php.net/manual/en/migration70.new-features.php#migration70.new-fe....

Looking into a solution.

And this:

  const MEMBER_NAME_REGEXP = '/^' .
    // First character must be "globally allowed". Length must be >=1.
    self::MEMBER_NAME_GLOBALLY_ALLOWED_CHARACTER_CLASS . '{1}' .
    '(' .
      // As many non-globally allowed characters as desired.
      self::MEMBER_NAME_INNER_ALLOWED_CHARACTERS . '*' .
      // If lenght is >1, then it must end in a "globally allowed" character.
      self::MEMBER_NAME_GLOBALLY_ALLOWED_CHARACTER_CLASS . '{1}' .
    // >1 characters is optional.
    ')?' .
    '$/u';

is triggering a parse error on PHP 5.5:

PHP Parse error:  syntax error, unexpected '.', expecting ',' or ';' in /Users/wim.leers/Work/d8/modules/jsonapi/src/JsonApiSpec.php on line 48

Apparently constant expressions are a PHP 5.6 feature: http://php.net/manual/en/migration56.new-features.php.

#27 failed precisely for the reasons mentioned in #15: the logic in HEAD was validating all parameter names, including official ones. We need to exclude official ones from "custom parameter" validation. We could apply member name validation to it, but it'd be pointless, since they're official query parameters anyway.

This fixes that.

I also took the opportunity to massively simplify \Drupal\jsonapi\Access\CustomParameterNamesAccessCheck::validate().

This makes the patch work in 5.5 and 5.6 too.

+++ b/tests/src/Unit/JsonApiSpecTest.php
@@ -37,7 +37,7 @@ class JsonApiSpecTest extends UnitTestCase {
-    $data['unicode-above-u+0080-highest-allowed'] = ["12\u{10FFFF}", TRUE];
+    $data['unicode-above-u+0080-highest-allowed'] = ["12<U+10FFFF>", TRUE];

This is for both PHP 5.5 and PHP 5.6.

All other changes are only necessary for PHP 5.5.

P.S.: you can't quote that line of the diff directly, because drupal.org still runs on Drupal 7, which does not accept 4-byte Unicode characters.

#22:

1. Technically this adds some coupling which didn't existed before. Not sure this is really needed.

   Hm… I guess that's true. Also: I don't see what the point of this is anymore. I think I did it because the test coverage was not setting this route requirement or something like that. In any case: agreed, removed.

2. Done.

#35:

1. Heh, #34 already did that :)
2. :)
3. :)

Did a final round of clean-up:

1. added the official/reserved query parameter names to the CustomQueryParameterNamesAccessCheckTest test coverage
2. clarified/corrected docs
3. renamed _json_api_custom_parameter_names to _jsonapi_custom_query_parameter_names
4. renamed CustomParameterNamesAccessCheck to CustomQueryParameterNamesAccessCheck

And just some final nits.

It's failing because it's already committed.

I figured @e0ipso would just revert his commit for this issue and then commit the now-RTBC patch.

Yay, thanks!

This unblocked #2829328: Clean up Drupal\jsonapi\Access\CustomParameterNames and make it follow the spec more closely.