Code error url fragment: wrong array key: key #fragment should be fragment

A patch to create a test that fails when run with the original code and succeeds when the bug has been fixed, as required in #8.

The new test testAddCheckToUrlForTrustedRedirectResponseWithFragment is an altered clone of testAddCheckToUrlForTrustedRedirectResponse of the same file. See #2.

The last submitted patch, 13: 3331870-2.patch, failed testing. View results →

Patch 3331870-2.patch fails testing as expected with the original code,

There was 1 failure:

1) Drupal\Tests\user\Unit\UserAuthTest::testAddCheckToUrlForTrustedRedirectResponseWithFragment
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'https://site.com?check_logged_in=1#a_fragment'
+'https://site.com?check_logged_in=1'

Fragment is missing.

Could apply the patch 3331870-1.patch → of #11 successfully. Resolves the problem.

Patch 3331870-3.patch = 3331870.patch → . 3331870-2.patch → . The simplest from @PrabuEla's patches 3331870.patch and 3331870-1.patch is chosen.

@Chris64, thanks for working on this issue and adding a test. Whenever we work on a patch, we leave it to others to review and eventually mark it RTBC. We don't set a patch we have changed to RTBC. You can read more about the RTBC status → or refer to the Contributor Guide → and apply the filter 'Drupal core'. That will show the available tasks and following the links provide the details.

I have updated the remaining tasks in the Issue Summary and setting to NW for those steps.

Also, what is the effect of this bug? Is it reproducible from the UI?

I have not reviewed the patch.

Thanks you for your answer @quietone. Of course for the status. A confusion between the work on my patch -3 and my work on the @PrabuEla's patch -1. The least status was the good one. Wrong for me.

Does the bug have some effects? Yes, we found it this way. But I passed details. For details, when we login we don't want to be at the top of the page. A demand from some users. We do this using a fragment. However it is lost along the way. Due to this bug.

To see the problem and its solution a simple way could be to clone and transform one file.

• Copy core/modules/user/src/Authentication/Provider/Cookie.php in your module, in the folder ./src/EventSubscriber with a name MyModuleUserResponseSubscriber.php.
• Copy the joint patch test-3331870.patch in your module's folder.
• Change all MyModule name to your module name in the patch and in the file name.
• Apply this altered patch.
• (Remark: the model file is the target of the issue's patch.)
• Make drush cr. Then logout and login. No fragment in the url.
• Apply @PrabuEla's patch 3331870.patch. Make drush cr, logout and login. There is a fragment.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

3331870-4.patch is the same than 3331870-3.patch. Used to bypass the bot who put NR to NW.

I wonder if this is going to be an issue

$event_mock = $this->createMock(ResponseEvent::class);

'final' classes cannot be doubled from my IDE.

The given line exists in the initial file. Even though it is used in our new test.

ResponseEvent is a Symfony class and not a Drupal class. In the file there is,
use Symfony\Component\HttpKernel\Event\ResponseEvent;
An alternative may be,
$event_mock = $this->createMock('Symfony\Component\HttpKernel\Event\ResponseEvent');
However ::class could be used for symfony class with createMock. The two uses exist in Drupal tests.

Makes sense. Wasn't breaking anything just one of those things PHPStorm was flagging.

Moving to NW for the issue summary update though along with what @quietone was saying #19.

The proposed resolution has two options? Document which one is being used and why.
Provide a fail patch and a success path

This seems to be a duplicate of 🐛 User login redirection missing fragment value Closed: duplicate

@ulaire, yes it seems!: same patch for Cookie.php file. However here there is a test. Useful.

@smustgrave, yes. See #17 for the choice:

The simplest from @PrabuEla's patches 3331870.patch and 3331870-1.patch is chosen.

(The choice of the simplest. If not right we have to give some arguments. May be weak, but much better that the existing one. Some thing between bad and perfect.)
See #13-15 for a failing test and #17 for a success test.

Could you please update the issue summary though. Helps reviewers and committers

Done: issue summary updated.

Triggering 10.1.x tests also.

Think the fix needs to be relooked at as this can't go into D10 as it causes test failures.

Ran locally also and got the same failure as https://www.drupal.org/pift-ci-job/2618515 →

... Yes, there is a problem with Drupal 10: test fails.

@smustgrave in fact you are absolutely right with #25! And the problem comes from here.

Error message: from file UserAuthTest.php

PHPUnit\Framework\MockObject\ClassIsFinalException: Class "Symfony\Component\HttpKernel\Event\ResponseEvent" is declared "final" and cannot be doubled

This file is different between Drupal 10 and 9: this problem is solved. Hence, we get our test doing the same from the originally UserAuthTest.php file version for Drupal 10 than for Drupal 9.

Thus two different patches for Drupal 10 and Drupal 9 due to the test.

A fail patch for Drupal 10.

Patch fails as expected:

There was 1 failure:

1) Drupal\Tests\user\Unit\UserAuthTest::testAddCheckToUrlForTrustedRedirectResponseWithFragment
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'https://site.com?check_logged_in=1#a_fragment'
+'https://site.com?check_logged_in=1'

To have a success patch. To prevent bot on fail + NR. D10-8 == D10-7.

Good job figuring that out!!

Committed 6b9d678 and pushed to 10.1.x. Thanks!

Keeping open for 10.0.x and 9.5.x backport.

10.0.x was a known random failure but 9.5.x needs a new patch.

A last patch for 9.5. Same as patch 4.

Backported to 10.0.x and applied #46 to 9.5.x

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

As said on the duplicated issue https://www.drupal.org/project/drupal/issues/3278692 🐛 Response loses URL fragment after adds check logged in to url Closed: duplicate

@david.muffley
This fixes the issue for my site, and you came to the same conclusion I did; $options['#fragment'] is incorrect as $options['fragment'] (no '#') is what's set through UrlHelper::parse().

However I question why this function is hand-crafting the URL rather than using Url::fromUri(...)->toString(). There aren't any comments explaining that choice so I have to assume it's just a mistake, which indirectly caused this bug in the first place.

Viewed @Eduardo Morales Alberti, I go here.