PHP 5.6 introduced constant expressions (see https://www.php.net/manual/en/migration56.new-features.php). This is something the JSON:API module wanted to use, but could not, because it had to support PHP 5.5 too. Drupal 8.8.0 having dropped support for PHP 5.5 allows us to use it after all.

Release notes snippet

JsonApiSpec::RESERVED_QUERY_PARAMETERS has changed from a string to an array.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Status: Active » Needs review
FileSize
2.84 KB
Wim Leers’s picture

Issue summary: View changes
Krzysztof Domański’s picture

Status: Needs review » Needs work

Due to coding standards, I change to "Needs work".

1. Two spaces after "=".
const MEMBER_NAME_REGEXP = '/^' .

2. A comma should follow the last multiline array item.

@@ -97,7 +99,7 @@ public static function isValidMemberName($member_name) {
     'sort',
     'page',
     'fields',
-    'include'
+    'include',
   ];

3. The following code is more readable, but coding standards report an error here... "String concat is not required here; use a single string instead".

self::MEMBER_NAME_GLOBALLY_ALLOWED_CHARACTER_CLASS . '{1}' .
'(' .

  (...)

')?' .
'$/u';

We should decide whether more readable code (add @codingStandardsIgnoreStart)

// @codingStandardsIgnoreStart
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 length > 1, then it must end in a "globally allowed" character.
  self::MEMBER_NAME_GLOBALLY_ALLOWED_CHARACTER_CLASS . '{1}' .
  // >1 characters is optional.
  ')?' .
  '$/u';
// @codingStandardsIgnoreEnd

or better coding standards

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 length > 1, then it must end in a "globally allowed" character.
  self::MEMBER_NAME_GLOBALLY_ALLOWED_CHARACTER_CLASS . '{1}' .
  // >1 characters is optional.
  ')?$/u';
Hardik_Patel_12’s picture

Status: Needs work » Needs review
FileSize
928 bytes
2.69 KB

Resolving coding standards as suggested in #4.

Krzysztof Domański’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 3100591-5.patch, failed testing. View results

Krzysztof Domański’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated test failure.

  • catch committed 2f2dc41 on 9.0.x
    Issue #3100591 by Hardik_Patel_12, Wim Leers, Krzysztof Domański:...
catch’s picture

Version: 8.9.x-dev » 9.0.x-dev
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed
Issue tags: +9.0.0 release notes
+++ b/core/modules/jsonapi/src/JsonApiSpec.php
@@ -71,33 +84,19 @@ class JsonApiSpec {
 
   /**
    * The reserved (official) query parameters.
-   *
-   * @todo When D8 requires PHP >= 5.6, convert to an array.
    */
-  const RESERVED_QUERY_PARAMETERS = 'filter|sort|page|fields|include';
+  const RESERVED_QUERY_PARAMETERS = [
+    'filter',
+    'sort',
+    'page',
+    'fields',
+    'include',
+  ];
 

This changes the value of a constant. I can't imagine anyone is relying on it, but probably makes this a 9.x-only change.

Added a release notes snippet just in case, I feel like a change record would be overkill here though.

Committed 2f2dc41 and pushed to 9.0.x. Thanks!

Wim Leers’s picture

#10:

 * @internal JSON:API maintains no PHP API since its API is the HTTP API. This
 *   class may change at any time and this will break any dependencies on it.
…
class JsonApiSpec {

🙂

I'm fine with this being 9.x-only.

Thanks for the thoughtful comment, @catch!

xjm’s picture

Issue tags: -9.0.0 release notes

@catch and I agreed that this is too minor to mention in the release notes.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.