Move auth_redirects ECA to its own recipe, or create a drupal_cms_eca recipe

No technical objections but I would like buy-in from Pam before we do this refactoring.

Sounds like a reasonable proposal to me, wouldn't be a big deal either.

This seems fine, I think it needs to be updated though now that we are removing the dashboard redirect.

I'll take this on.

In principle this seems okay but I'm a little conflicted that it should be its own recipe. It's a very small piece of the overall Drupal CMS puzzle, and frankly these redirects are things that should be fixed in core anyway. Adding more recipes increases complexity and maintenance burden, albeit not by much...but it adds up.

Is there any chance we could hold off on this until after release, and split out this feature into its own recipe if there is demand for that?

I'm holding back on this. I just can see the point from @thejimbirch that the recipe would be interesting also outside of Drupal CMS. For that, I can offer to make a copy of that recipe available stand-alone, if that helps?

In principle this seems okay but I'm a little conflicted that it should be its own recipe. It's a very small piece of the overall Drupal CMS puzzle, and frankly these redirects are things that should be fixed in core anyway. Adding more recipes increases complexity and maintenance burden, albeit not by much...but it adds up.

It is a very small piece of the Drupal CMS, but one that provides a foundation and example for using ECA instead of custom code or more contributed modules that needs to be maintained.

It indeed adds some maintenance burden for Drupal CMS , but I worry that that line of thinking will make the base Drupal CMS recipe too opinionated. The fact that ECA's are saved as config entities make them shareable and repeatable. Allowing this small piece of functionality to be its own package would benefit and lower the maintenance cost for the community with being able to require and use instead of copying and pasting.

In the Recipe Author Guide's Guidelines for interoperable recipes it is recommended to

Put reusable config in its own recipe.

As a rule, where a config entity is applicable to many different use
cases, it should go in its own recipe....

As per the brief proposal and discussion in #3482188-2: Simplify 403 redirecting by setting /user/login as the 403 page → , let's go ahead with this. Please call the new recipe drupal_cms_authentication, since we'll add other stuff to it that makes the authentication experience better.

To prove that point, let's also move the dependency on login_emailusername into this new recipe as well.

The MR contains these changes:

• Move the ECA authentication into its own recipe
• Use Drupal core's mechanism for 403 redirect handling
• Also move the login by email module into that new recipe

The ECA model got cleaned up, and it now only contains the redirect after logout. I wonder, should we keep the redirect after login to the dashboard in there as well? It doesn't hurt, since the hard-coded redirect from the dashboard module overrides that anyway. But for those who use this recipe elsewhere, it might be a helpful component.

The 403 redirect test had to be changed, as core manages this such that the url still contains /admin and not /user/login, although we're obviously on the login page. I've changed the assertion such that it checks for the "Log in" button.

The failing test is about the anti spam recipe not being available, and I wonder if that should be a dependency here, too? If not, we need to remove its permissions from the test.

Seems pretty solid - just needs a little more fine-tuning.

I've rebased this MR and adopted all suggested changes except for one. I couldn't find the right methods on how to test for the response code or the form_id. @phenaproxima could you either give me a hint or add those directly? It's the only thread left in the MR, all others are resolved.

Okay, left a few hints on the MR. :)

Please also add drupal_cms_authentication to the job matrix in .gitlab-ci.yml's "run PHP tests" job. Otherwise the test won't run!

Completed all feedback threads and tests are all green.

Only one complaint here, which is that WebDriverWebAssert makes no sense in the context of this test.

Otherwise I think this is good to go.

Resolved those remaining threads.

Not quite...

Oh, one more thing - please add this new recipe to the PHPUnit job matrix in .gitlab-ci.yml.

Add recipe to matrix.

As the recipe gets tested in isolation, the antispam modules are not loaded and hence we can't remove their permissions. Have removed that from the tests.

That makes sense to me. 👍

Happy with this; merging on green.

Merged into 0.x. This is a good idea - the better we can make the login experience, the better everyone off will be since this is a shareable and reusable component.

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