Problem/Motivation

Part of #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves)

Steps to reproduce

php -r 'var_dump(explode("|", NULL));'
PHP Deprecated:  explode(): Passing null to parameter #2 ($string) of type string is deprecated in Command line code on line 1

Deprecated: explode(): Passing null to parameter #2 ($string) of type string is deprecated in Command line code on line 1
array(1) {
  [0]=>
  string(0) ""
}

Proposed resolution

Ensure the result of getRequirement() is not passed to a string function when it is NULL.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

andypost’s picture

Status: Active » Needs review
FileSize
1.22 KB
1.23 KB

@alexpott the interdiff against META

daffie’s picture

Status: Needs review » Needs work
+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -383,10 +383,11 @@ public function collectRoutes(RouteCollection $collection) {
+    $route_formats = $no_route_format ? [] : explode('|', $route->getRequirement('_format'));

Can we change this line for better readability to:

  $route_formats = $route->hasRequirement('_format') ? explode('|', $route->getRequirement('_format')) : [];

To me the added variable "$no_route_format" just does not improve the overall readability.

andypost’s picture

Status: Needs work » Needs review

This variable used to prevent calling $route->hasRequirement('_format') twice

daffie’s picture

Status: Needs review » Needs work

All that the method call does is (from vendor/symfony/routing/Route.php):

    /**
     * Returns the requirement for the given key.
     *
     * @param string $key The key
     *
     * @return string|null The regex or null when not given
     */
    public function getRequirement($key)
    {
        return $this->requirements[$key] ?? null;
    }

To me the added variable does not increase the overall readability.

There is still one call the the method that can be replaced, if you want to replace them all:

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -383,10 +383,11 @@ public function collectRoutes(RouteCollection $collection) {
+    $view_route_formats = $view_route->hasRequirement('_format') ? explode('|', $view_route->getRequirement('_format')) : [];
andypost’s picture

Status: Needs work » Needs review

@daffie this is different route $view_route, the patch replaceing only calls to $route->hasRequirement('_format')

daffie’s picture

Status: Needs review » Reviewed & tested by the community

@andypost: You are right.

The patch looks good to me.

larowlan’s picture

+++ b/core/modules/rest/src/Plugin/views/display/RestExport.php
@@ -383,10 +383,11 @@ public function collectRoutes(RouteCollection $collection) {
+    $no_route_format = !$route->hasRequirement('_format');
+    $route_formats = $no_route_format ? [] : explode('|', $route->getRequirement('_format'));
...
+      && ($no_route_format || array_intersect($route_formats, $view_route_formats) != []);

I think this would read better if the variable was $has_route_format, i.e. we can keep both ternaries in the same order - default then fallback, rather than having them one each way.

Then we can just use !$has_route_format in the if statement

As it stands, it took a few passes to comprehend what was happening

Thoughts?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Please do all the \Symfony\Component\Routing\Route::getRequirement() fixes in #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves) in one issue.

alexpott’s picture

Also the use of $no_route_format and then !$no_route_format for double negatives is not at all readable. The original change in #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves) was better.

alexpott’s picture

Priority: Normal » Critical
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +PHP 8.1
FileSize
2.59 KB
3.06 KB

Here's the work as was from #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves) plus the other changes due to getRequirement() returning NULL.

alexpott’s picture

Title: \Drupal\rest\Plugin\views\display\RestExport::overrideApplies() causes deprecation errors on PHP 8.1 » \Symfony\Component\Routing\Route::getRequirement() causes deprecation errors on PHP 8.1 when it returns NULL
andypost’s picture

Component: views.module » routing system
Status: Needs review » Reviewed & tested by the community

Great to fix all places same time!

the use of $no_route_format and then !$no_route_format for double negatives is not at all readable.

This is what I used to try prevent - readability of ! always hard

  • catch committed cd6b58e on 9.3.x
    Issue #3239553 by andypost, alexpott, daffie, larowlan: \Symfony\...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed cd6b58e and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

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