Problem/Motivation

Since symfony/http-kernel 5.3: Passing null as $message to "Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException::__construct()" is deprecated, pass an empty string instead.

The source commit is Deprecate passing null as $message or $code to exceptions https://github.com/symfony/symfony/commit/8e3058d95acc7d177516bfacce794f...

Steps to reproduce

Update to Symfony 5.3 as in #3161889: [META] Symfony 6 compatibility

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 3209619-test.patch149.87 KBlongwave

Issue fork drupal-3209619

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

longwave created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes

Added upstream commit reference to IS, all exceptions been changed

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Title: [Symfony 6] Passing null as $message to HTTP exception constructors is deprecated, pass an empty string instead » [Symfony 6] Passing null as $message to Symfony exception constructors is deprecated, pass an empty string instead

Thanks for finding the upstream commit, changed the title to match. Looks like the $code deprecation doesn't affect us.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
StatusFileSize
new 3209619-test.patch149.87 KB

Attached patch is latest #3161889: [META] Symfony 6 compatibility with two deprecation skips removed for this issue, in order to figure out what we need to change to solve this.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

We have a decision to make here: we can just cast NULLs to string when we throw exceptions, or we can go a bit further and remove NULL entirely, using the empty string instead of NULL to mean "no message" or "no reason".

In AccessAwareRouter:

      if ($access_result instanceof CacheableDependencyInterface && $request->isMethodCacheable()) {
        throw new CacheableAccessDeniedHttpException($access_result, $access_result instanceof AccessResultReasonInterface ? $access_result->getReason() : NULL);
      }
      else {
        throw new AccessDeniedHttpException($access_result instanceof AccessResultReasonInterface ? $access_result->getReason() : NULL);
      }

It is trivial to change the NULLs here to the empty string, but AccessResultReasonInterface::getReason() can also return NULL. The docs for this are:

  /**
   * Gets the reason for this access result.
   *
   * @return string|null
   *   The reason of this access result or NULL if no reason is provided.
   */
  public function getReason();

If follow Symfony here to only return strings (with the empty string meaning "no reason"), in the future we can simplify things like this as well:

  /**
   * Creates an AccessResultInterface object with isNeutral() === TRUE.
   *
   * @param string|null $reason
   *   (optional) The reason why access is neutral. Intended for developers,
   *   hence not translatable.
   *
   * @return \Drupal\Core\Access\AccessResultNeutral
   *   isNeutral() will be TRUE.
   */
  public static function neutral($reason = NULL) {
    assert(is_string($reason) || is_null($reason));
    return new AccessResultNeutral($reason);
  }

Personally I think this is the correct thing to do, because at the moment you can pass either NULL or "" as a reason and they mean the same thing.

catch’s picture

catch
he/him
Primary language English
commented
Priority: Normal » Critical

Bumping to critical since this blocks Symfony 6 adoption.

Deprecating passing null (and defaulting to empty string?) seems like a good idea, although probably for a follow-up?

longwave opened merge request !719

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review

Last commit in the MR is the one to review, the others are setting up SF 5.3.

daffie’s picture

daffie commented
Status: Needs review » Needs work

The changes in the last commit look good to me, only the testbot is not happy.
Also a number of related issues have landed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

Fixed some places where NULL was expected but we now have the empty string. I guess this is a BC break...

daffie’s picture

daffie commented
Status: Needs review » Needs work
  1. I see a lot of changes for which I do not see why they are related to the problem as stated in the IS. Could you explain how they are related.
  2. Could you post a MR or patch with the changes that you think should be committed to 9.3.

@longwave: I want to help to get this issue to land by reviewing only it is to me not clear how it all works.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

@daffie The MR also contains work from #3161889: [META] Symfony 6 compatibility in order to upgrade to Symfony 5.3 and prove that the deprecations are fixed and also get all the tests to pass. I tried to keep the commits atomic so they can be reviewed individually.

https://git.drupalcode.org/project/drupal/-/merge_requests/719/commits

5b24d166 - stop skipping the deprecations that are covered by this issue so tests will fail if we still use them
4c4bff9b - latest patch for Symfony 5.3 support
5fa65acf - second round of fixes
0261f8bb - first round of fixes

The other commits are previous versions of this patch and can be ignored for now, for example a94a14b3 was an earlier version of Symfony 5.3 but 657453fd reverts this again so effectively it can be ignored. Sorry if this is confusing - I will try to get tests to pass and then open a separate MR with the fixes only.

catch’s picture

catch
he/him
Primary language English
commented

Apologies for all the rebase noise, I ended up doing one thing, then changing my mind and doing another...

First of all tried a straight rebase of the branch, but there's quite a lot of conflicts with the Symfony 5 update parts, which meant it started turning into a rebuild of the branch instead of a rebase. About half way through I realised it'd be better to do that over on #3161889: [META] Symfony 6 compatibility.

So.. instead I've removed the Symfony 5 updates from the branch altogether, and this is now only the changes required for this patch (I hope, tests just started running) on the latest 9.3.x. Essentially what longwave mentions in #13.

If the changes here are green, then I think the next step is probably to refresh #3161889: [META] Symfony 6 compatibility, and then generate a combined patch (or separate MR if we want to) from the two issues, and post that here to demonstrate the change passing against Symfony 5 too.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

The MR is passing against 9.3.x now.

#3161889: [META] Symfony 6 compatibility now contains the latest diff from the MR here, so we can see how it does with an actual Symfony 5.4 update over there.

daffie’s picture

daffie commented

All the code changes look good to me and are RTBC for me.

The only problem that I have is that the change of the return value of getReason(). To me it will be a BC break. My question is how small the impact will be.

I cannot find any instances where the method is overriidden. See: http://grep.xnddx.ru/search?text=function+getReason%28%29&filename=.

I can find instances where in contrib NULL is returned instead of an empty string. Like: $access_result instanceof AccessResultReasonInterface ? $access_result->getReason() : NULL. Not sure that will case a BC break, only 4 instances in contrib. See: http://grep.xnddx.ru/search?text=-%3EgetReason%28%29+%3A+null&filename=.

I can find a couple of instances in contrib testing whih: $this->assertNull($access->getReason());. I found it only in the scheduled_transsitions module. See: http://grep.xnddx.ru/node/31758647.

No idea what custom modules are doing.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

I thought about the solution a bit more and for me it is RTBC.

I leave it up to the committer to decide if the small BC break is exceptable and that it does not break any existing sites out there. For me I am not a 100% sure that there are no sites that broken at least a small bit.
On the other side is the fact that we want to have Symfony 6 in D10. To me this too important to not do this issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

I think we have to take the tiny risk that this will break a site. We need this for PHP 8.1 compatibility too.

Committed 7c3b2861e1 and pushed to 9.3.x. Thanks!

  • alexpott committed 7c3b286 on 9.3.x 
    Issue #3209619 by longwave, daffie, catch: [Symfony 6] Passing null as $...

Status: Fixed » Closed (fixed)

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