Redirects not deleted on alias creation on monolingual sites

Attached a failing patch.

When I add a single path alias, it gets assigned the current English (only existing) language.
But if I use the node form, it gets assigned the UNDEFINED language.

The last submitted patch, 2: 3377195-2.only-test.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Ok. I get it it now. I think the correct fix is to always look for redirects in the given language *and* UND, because that's how lookups works as well. We can do that directly within redirect_delete_by_path().

1. +++ b/tests/src/Functional/RedirectPathAliasTest.php
   @@ -0,0 +1,68 @@
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected $profile = 'minimal';
   

   why do wee need minimal here?

2. +++ b/tests/src/Functional/RedirectPathAliasTest.php
   @@ -0,0 +1,68 @@
   +
   +    // Create a redirects.
   +    $redirect = Redirect::create();
   +    $redirect->setSource('/test-page');
   +    $redirect->setRedirect('<front>');
   +    $redirect->setLanguage(LanguageInterface::LANGCODE_NOT_SPECIFIED);
   +    $redirect->setStatusCode(301);
   +    $redirect->save();
   

   lets create the redirect in the test, we might or might not need that in other methods.

   I thought we have existing test coverage for this, but can't find it.

   Would be great to extend on it a little bit, add a few more redirects, one in the matching language that should be deleted too and one with a different path that shouldn't be deleted.

   For bonus points, make it multilingual, create an alias with the same path and another language that doesn't get deleted.

Attached patch with extended tests.
I think the fix should be calling redirect_delete_by_path() with undefined language when the path alias language is the default.

You said lookups works in the given language *and* UND, so I'm wondering if it should happen every time. Where are the lookups made? Maybe a @see on the changed logic could be useful.

The last submitted patch, 5: 3377195-5.only-test.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 5: 3377195-5.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

See \Drupal\redirect\RedirectRepository::findMatchingRedirect().

And I don't agree with default language condition, because default language or not has no relevance to the lookup. we always look up specific language first, UND second.

Drupal\Tests\redirect_404\Kernel\Fix404RedirectCronJobTest is failing on HEAD too, so shouldn't affect this patch.

Delete the redirect unconditionally for LANGCODE_NOT_SPECIFIED. We might want to move that to the query avoiding two different queries. Didn't do that because the logic for $match_subpaths_and_redirect might change too and I'm not aware when and how that's used.

The last submitted patch, 9: 3377195-9.only-test.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 9: 3377195-9.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The failing test in unrelated, so setting this as NR.

The last submitted patch, 9: 3377195-9.only-test.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

ishanghosh27 → made their first commit to this issue’s fork.

ishanghosh27 → closed merge request !59

Vishal Prasad → made their first commit to this issue’s fork.

Applied Patch - 3377195-9.patch by penyaskito → . Working as expected. Reviewed and tested at my end.

Vishal Prasad → closed merge request !60

@Berdir I have tested the 3377195-9.patch by @penyaskito and it works good to me too,
i think we can create a MR and Merge it.

I don't understand why there are two closed MR's here with no changes. The patch should be converted to a merge request so that we can run current tests against it.

@berdir I had accidentally opened an MR as this was the first time I was contributing to open source and got confused about what was required for this issue (Testing the patch). Later, I closed the MR and tested the existing patch. Hope this clarifies the query.