User routes alter in custom module throwing error on "_format"

Created on 11 May 2023, almost 2 years ago
Updated 19 September 2024, 6 months ago

Problem/Motivation

Deprecated function: explode(): Passing null to parameter #2 ($string) of type string is deprecated in Drupal\serialization\EventSubscriber\UserRouteAlterSubscriber->onRoutingAlterAddFormats() (line 55 of core/modules/serialization/src/EventSubscriber/UserRouteAlterSubscriber.php).

The deprecation notice can occur if some other code has altered and removed the _format requirement on one of the 'user.login_status.http', 'user.login.http', 'user.logout.http', or 'user.pass.http' routes.

Steps to reproduce

Have another module alter one of the routes to remove _format requirement. The other module has to come before the serialization module.

See the proposed test case in the merge request.

Steps to reproduce issue,

1. Create custom module having the logic to alter the Route.
2. Altering RouteSubscriberBase as below.

<?php

namespace Drupal\custom_module\Routing;

use Drupal\Core\Routing\RouteSubscriberBase;
use Symfony\Component\Routing\RouteCollection;

/**
 *
 */
class RouteSubscriber extends RouteSubscriberBase {

  /**
   * Disable access to cohesion entities if assets have not been imported
   * or "Use Site studio" option is set to 'disable'
   * {@inheritdoc}.
   */
  protected function alterRoutes(RouteCollection $collection) {

    if ($route = $collection->get('user.pass.http')) {
      $route->setRequirements([]);
      $route->setRequirement('_access', 'FALSE');
    }
  }

}

3. Clear the cache, Access the route user/password
4. Getting below error

Deprecated function: explode(): Passing null to parameter #2 ($string) of type string is deprecated in Drupal\serialization\EventSubscriber\UserRouteAlterSubscriber->onRoutingAlterAddFormats() (line 55 of /var/www/html/docroot/core/modules/serialization/src/EventSubscriber/UserRouteAlterSubscriber.php)
#0 /var/www/html/docroot/core/includes/bootstrap.inc(164): _drupal_error_handler_real(8192, 'explode(): Pass...', '/var/www/html/d...', 55)
#1 [internal function]: _drupal_error_handler(8192, 'explode(): Pass...', '/var/www/html/d...', 55)
#2 /var/www/html/docroot/core/modules/serialization/src/EventSubscriber/UserRouteAlterSubscriber.php(55): explode('|', NULL)

Proposed resolution

Check the _format exists and not empty.
Confirm $route->getRequirement('_format') has value before passing for explode().

Remaining tasks

Needs review.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

None.

🐛 Bug report
Status

Needs review

Version

11.0 🔥

Component
Serialization 

Last updated about 2 months ago

Created by

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @aduthois
  • Please post the stack trace.

  • Also, I don't know what this is about. Can you add some more detail there please?

    Route '/user/password' no '_format' requirement

  • explode('|', $route->getRequirement('_format')) with null when $route_name is 'user.pass.http' :
    public function onRoutingAlterAddFormats(RouteBuildEvent $event) {
    $route_names = [
    'user.login_status.http',
    'user.login.http',
    'user.logout.http',
    'user.pass.http',
    ];
    $routes = $event->getRouteCollection();
    foreach ($route_names as $route_name) {
    if ($route = $routes->get($route_name)) {
    $formats = explode('|', $route->getRequirement('_format'));
    $formats = array_unique(array_merge($formats, $this->serializerFormats));
    $route->setRequirement('_format', implode('|', $formats));
    }
    }
    }

  • Status changed to Postponed: needs info almost 2 years ago
  • This issue needs some editing to be understandable to contributors.

  • 🇫🇷France Manuel.loth

    Hello,
    I have the same issue.
    a call to drupal_flush_all_caches() results in the following warning :

    Deprecated function: explode(): Passing null to parameter #2 ($string) of type string is deprecated in Drupal\serialization\EventSubscriber\UserRouteAlterSubscriber->onRoutingAlterAddFormats() (line 55 of core/modules/serialization/src/EventSubscriber/UserRouteAlterSubscriber.php).
    
    Drupal\serialization\EventSubscriber\UserRouteAlterSubscriber->onRoutingAlterAddFormats(Object, 'routing.route_alter', Object)
    call_user_func(Array, Object, 'routing.route_alter', Object) (Line: 111)
    Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'routing.route_alter') (Line: 189)
    Drupal\Core\Routing\RouteBuilder->rebuild() (Line: 83)
    Drupal\Core\ProxyClass\Routing\RouteBuilder->rebuild() (Line: 518)
    drupal_flush_all_caches() (Line: 316)
    Drupal\bofip_za_documents\Form\ActuAlaUneForm->submitForm(Array, Object)
    call_user_func_array(Array, Array) (Line: 114)
    Drupal\Core\Form\FormSubmitter->executeSubmitHandlers(Array, Object) (Line: 52)
    Drupal\Core\Form\FormSubmitter->doSubmitForm(Array, Object) (Line: 597)
    Drupal\Core\Form\FormBuilder->processForm('actu_ala_une_form', Array, Object) (Line: 325)
    Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
    Drupal\Core\Controller\FormController->getContentResult(Object, Object)
    call_user_func_array(Array, Array) (Line: 123)
    Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 580)
    Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
    Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
    Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 163)
    Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 74)
    Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
    Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
    Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
    Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
    Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
    Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
    Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 51)
    Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 686)
    Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
    

    So it seems to be caused by the absence of parameter '_format' in the 'user.pass' route in web/core/modules/user/user.routing.yml, as can be seen in the code posted above that I quote again :
    \Drupal\serialization\EventSubscriber\UserRouteAlterSubscriber::onRoutingAlterAddFormats, line 55 :

            $formats = explode('|', $route->getRequirement('_format'));
    

    Apparently, _format is now a required parameter for all routes, but it is not present in user.pass.

    So the proposed resolution should be to fix the route by adding this parameter.

  • I cannot reproduce this. I installed Drupal 10.0.x on PHP 8.2 with full error reporting.

    Are the affected sites running patches on Drupal Core?

    onRoutingAlterAddFormats is only evaluating these routes:

    $route_names = [
          'user.login_status.http',
          'user.login.http',
          'user.logout.http',
          'user.pass.http',
        ];
    

    user.pass is not one of them. user.pass.http is on of them, but it has the _format configured.

  • Status changed to Active almost 2 years ago
  • The problem came from a contrib ldap module which overrides the 'Requirements' parameter of user.pass.http.
    It would still be useful to check the 'explode()' method for nulls. THANKS

  • But those four routes require it. Why would it be good to silently work around the missing value?

  • Status changed to Closed: works as designed almost 2 years ago
  • ok i understand close this issue

  • First commit to issue fork.
  • Status changed to Needs review 8 months ago
  • 🇩🇰Denmark arnested

    I'm reopening this and adding a suggested fix.

    I'm running into this issue as well.

    My use case is that I want to deny access to /user/password (we use an external login provider).

    In a RouteSubscriber::alterRoutes() we do:

        if ($route = $collection->get('user.pass.http')) {
          $route->setRequirements([]);
          $route->setRequirement('_access', 'FALSE');
        }
    

    We clear all existing requirements and then set access to FALSE because we want to be sure no other requirements interfere.

    But then there is no format on the route (and none should be needed).

  • Status changed to Needs work 8 months ago
  • The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • Status changed to Needs review 8 months ago
  • 🇩🇰Denmark arnested

    Fixed phpstan issue.

  • Status changed to Needs work 8 months ago
  • 🇺🇸United States smustgrave

    believe the conversation around 8-10 still applies. Seems like this could be an issue with some contrib module.

  • Pipeline finished with Failed
    8 months ago
    Total: 160s
    #229135
  • Pipeline finished with Success
    8 months ago
    Total: 538s
    #229143
  • Status changed to Needs review 8 months ago
  • 🇩🇰Denmark arnested

    OK, I got a test that triggers the deprecation notice.

    The deprecation notice occurs when some other module/code has altered away the _format on, e.g., the user.pass.http route.

    But now I'm unsure what the serialization module should actually do when coming across such a route. Should it still alter the route and add json|xml as _format, or should it refrain from adding an altered format back in?

    My first thought was to not add it, but on a second thought, I think it should. If you really want the format gone, you should probably make sure your event subscriber runs after the serialization module's event subscriber.

    Issue summary updated.

  • Pipeline finished with Success
    8 months ago
    Total: 900s
    #229152
  • Pipeline finished with Success
    8 months ago
    #229161
  • 🇩🇰Denmark arnested

    arnested changed the visibility of the branch 3359649-deprecated-function-explode to hidden.

  • 🇮🇳India arunkumark Coimbatore

    I was able to replicate the issue from my side. I followed the below steps to reproduce,

    1. Installed custom module having the logic to alter the Route.
    2. Altering RouteSubscriberBase as below.
    3. Clear the cache, Access the route user/password
    4. Getting below error

    Deprecated function: explode(): Passing null to parameter #2 ($string) of type string is deprecated in Drupal\serialization\EventSubscriber\UserRouteAlterSubscriber->onRoutingAlterAddFormats() (line 55 of /var/www/html/docroot/core/modules/serialization/src/EventSubscriber/UserRouteAlterSubscriber.php)
    #0 /var/www/html/docroot/core/includes/bootstrap.inc(164): _drupal_error_handler_real(8192, 'explode(): Pass...', '/var/www/html/d...', 55)
    #1 [internal function]: _drupal_error_handler(8192, 'explode(): Pass...', '/var/www/html/d...', 55)
    #2 /var/www/html/docroot/core/modules/serialization/src/EventSubscriber/UserRouteAlterSubscriber.php(55): explode('|', NULL)

    After checkout to the Fix MR.

    Am good on moving to RTBC

    LGTM

  • Status changed to RTBC 8 months ago
  • 🇮🇳India arunkumark Coimbatore
  • 🇳🇿New Zealand quietone

    I read the issue summary, the comments and the MR (not a code review). There are few things still to do, not in any particular order.

    1. The issue title is an error message. The issue title should be a a description of what is being fixed here. See https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...
    2. The proposed resolution section should be be a description of the change so that a reviewer knows what should be implemented.
    3. I see that this was closed once as working as designed. This was re-opened and information about that was asked for in #17 but I don't see an answer. Have I missed something?
    4. I also don't see any evidence of a code review.

  • 🇮🇳India arunkumark Coimbatore

    Updated issue summary and title as recommended in #23

  • Status changed to Needs work 7 months ago
  • 🇬🇧United Kingdom catch

    The MR still has unresolved feedback.

  • Status changed to Needs review 7 months ago
  • 🇩🇰Denmark arnested

    I fixed the comment addressed in the merge request.

  • Pipeline finished with Failed
    7 months ago
    Total: 476s
    #237285
  • Status changed to RTBC 7 months ago
  • 🇺🇸United States smustgrave

    Feedback appears to be addressed.

  • 🇳🇿New Zealand quietone

    This is better shape now, thanks.

    However, I still don't see that someone has actually looked and the code and reviewed it.

  • Status changed to Needs work 7 months ago
  • 🇬🇧United Kingdom catch

    couple of comments on the MR.

  • Status changed to Needs review 7 months ago
  • Pipeline finished with Success
    7 months ago
    Total: 511s
    #249287
  • Status changed to RTBC 7 months ago
  • 🇺🇸United States smustgrave

    https://git.drupalcode.org/issue/drupal-3359649/-/jobs/2397062 still shows test coverage.

    See code change has been addressed and 1 question answered.

  • Status changed to Needs work 6 months ago
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    Made a suggestion for a cleaner fix on the MR.

  • First commit to issue fork.
  • Pipeline finished with Failed
    6 months ago
    Total: 594s
    #286304
  • Assigned to shalini_jha
  • Pipeline finished with Failed
    6 months ago
    Total: 1502s
    #286931
  • Pipeline finished with Failed
    6 months ago
    Total: 408s
    #287097
  • Issue was unassigned.
  • Status changed to Needs review 6 months ago
  • 🇮🇳India shalini_jha

    "I have updated the code based on PR feedback and fixed the pipeline for the failing test after modifying UserRouteAlterSubscriber.php. However, I'm still facing pipeline failures, which may be due to the MR needing a rebase. I attempted to rebase, but it shows that everything is up to date. Moving forward with the NR, please guide if I'm missing something in the rebase process."

  • 🇺🇸United States smustgrave

    Nightwatch keeps failing, needs to be investigated

  • Pipeline finished with Success
    5 months ago
    Total: 917s
    #294231
  • 🇩🇰Denmark arnested

    I couldn't make any sense out of the nightwatch failure.

    I did a rebase on 11.x and now the tests are all green.

  • 🇺🇸United States smustgrave

    Ran the test-only feature

    There was 1 failure:
    1) Drupal\Tests\serialization\Kernel\UserRouteAlterTest::testUserAlteredRouteWithoutFormat
    user.pass.http route does not have "_format" requirement
    Failed asserting that an array does not have the key '_format'.
    /builds/issue/drupal-3359649/core/modules/serialization/tests/src/Kernel/UserRouteAlterTest.php:73
    --
    2 tests triggered 1 PHP deprecation:
    1) /builds/issue/drupal-3359649/core/modules/serialization/src/EventSubscriber/UserRouteAlterSubscriber.php:55
    explode(): Passing null to parameter #2 ($string) of type string is deprecated
    Triggered by:
    * Drupal\Tests\serialization\Kernel\UserRouteAlterTest::testUserAlteredRoute
      /builds/issue/drupal-3359649/core/modules/serialization/tests/src/Kernel/UserRouteAlterTest.php:51
    * Drupal\Tests\serialization\Kernel\UserRouteAlterTest::testUserAlteredRouteWithoutFormat
      /builds/issue/drupal-3359649/core/modules/serialization/tests/src/Kernel/UserRouteAlterTest.php:64
    FAILURES!
    

    Which showed covered, and both tests failed nice!

    Reviewing issue summary and that appears complete. Has steps to reproduce and solutions matches what's in MR.

    Reviewing code changes in MR and addition of the check for _format makes sense, rest appears around the test coverage.

    LGTM

  • Pipeline finished with Success
    5 months ago
    #302929
  • Pipeline finished with Running
    5 months ago
    #302943
    • alexpott committed 059150ef on 10.3.x
      Issue #3359649 by arnested, shalini_jha, alexpott, arunkumark, cilefen,...
    • alexpott committed 19853cff on 10.4.x
      Issue #3359649 by arnested, shalini_jha, alexpott, arunkumark, cilefen,...
    • alexpott committed 3b606077 on 11.0.x
      Issue #3359649 by arnested, shalini_jha, alexpott, arunkumark, cilefen,...
    • alexpott committed a6a40068 on 11.x
      Issue #3359649 by arnested, shalini_jha, alexpott, arunkumark, cilefen,...
  • 🇬🇧United Kingdom alexpott 🇪🇺🌍

    Committed and pushed to 11.x,11.0.x, 10.4.x and 10.3.x. Thanks!

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

Production build 0.71.5 2024