Restore link to add a redirect on node edit forms

Confirming this works and is sorely needed in the latest version of this module for content authors to easily add redirects from the "source" content inside the vertical tab! Thanks for the patch.

Yep, patch in #2 is working for me too - many thanks.

NOTE: Because this patch prefills the redirect form's "To" field, it's much more likely that you'll bump into this Path field focus bug.

Thanks for patch @mariacha1
I cleanup up module file and made sure it passes phpcs linting.

Do we want to make it more like D7, e.g. more prominent button and not opening in new window?

@VladimirAus To make it easier for the maintainers to review patches, I suggest we fix the phpcs linting in #2957751: Maintaining drupal coding standards

With regards to the current patch: in #2931770: Improve usability of redirect operation links in node form Berdir commented the user would lose any customized input in the node form. Perhaps we can implement a modal where the redirect can be added (or load the form through AJAX?), so the node form is still available after adding a redirect?

@idebr Thanks for your input. Will have a look at coding standards issue there.
As of input from all my clients: they would like to see Drupal 7 functionality:
* click button > redirect to `add redirect` page > save and redirect back to node edit.

I agree this would be a very important feature, which is currently missing in the 8.x branch.

Indeed, patch #6 includes a bunch of unrelated changes that (while worth fixing) make it harder to review and commit this feature. Most of that needs to move to #2957751: Maintaining drupal coding standards. Setting to 'Needs work' to simplify this back to a basically working version of #2.

Meanwhile, I'm sad that I keep helping add this feature to this module (or its predecessor), only to see it removed again during porting to new versions. ;) I first worked on this at #168877: Path-redirect as an option on the node form (add or edit). See also #1212012: Define a redirect while adding a node. Once this lands, can we please leave this functionally in the module so we don't have to keep re-re-adding it every few years? ;) Clearly people want this to keep working.

Now that redirects are entities, I wonder if inline entity form could be used to help make this even better. Maybe it's not a great fit, since that's mostly a widget for entity_reference fields (which this isn't), but perhaps there are parts of it that could be re-used. That's probably a bit out of scope and should be a follow-up issue, so as not to complicate this issue. Better to get something less slick working and committed, and then we can improve it later.

That said, the entity form at admin/config/search/redirect/add is so simple (3 fields and a submit button), and one of those fields isn't needed in this case (the 'To' target). So to let people add redirects inline on the entity form is really injecting 2 fields. That might be quite trivial to do directly via #ajax in the entity form.

Anyway, I'll work on a new version of the patch. Stay tuned.

Upon further testing, the previous patches both fail in that the destination they set assumes a site is not installed in a subdirectory. We need to use Url::fromRoute('<current>') to get a working destination.

So this is basically patch #2, plus fixing destination, plus setting a 'button' class on the link (so admin themes make it look like a more prominent action button). Interdiff relative to #2 attached. Please ignore #6.

Thanks,
-Derek

Whoops, sorry. I noticed a trivial code style bug in #11. Use this instead.

And why use 'node' in the user-facing UI when we can load the node type and get the human-readable label, instead?

... or if you prefer %type as the placeholder (which is what it seems like core tends to do), use this.

Oh, look, there's a redirect.destination service. ;) Even better. Learn something new every day. ;)

p.s. Most of the other (unrelated) fixes from #6 are now solved at #2961293: Convert module to use short array syntax (new coding standard). That's a giant patch to fix *all* references to array(), and hopefully will be landing soon.

More accurate title ("restore" since we've had this in D5, D6 and D7 but it was removed (apparently without reason) from D8) and adding more related issues.

It wasn't removed as much as never implemented. D8 was done basically from scratch.

I think this should have a test, we AFAIK have existing tests for listing the redirects, adding one more check there should be relatively easy.

What kind of test? Something that ensures that the link appears? I assume there's already test coverage of the add form itself.

Duly noted on removed vs never implemented in D8. Thanks for clarifying.

Clicking on the link and testing that the add form is correctly pre-filled with the redirect target.

Are you talking about the (deprecated) src/Tests/RedirectUITest.php WebTestBase tests that are clicking around the UI? I didn't see anything in there about the redirect UI on node edit forms, other than testing that automatic redirects are added when you change the path alias. Is that what you meant?

Instead of adding yet more logic in there which will have to be ported in the future, it seemed better to add a whole new BrowserTestBase test class for this. Porting over the deprecated node edit UI testing from WebTestBase to this class is left as an exercise for the interested reader in a follow-up issue. ;)

While I was at it, I added an @todo about perhaps using off_canvas or something via #2931770: Improve usability of redirect operation links in node form. For now, to avoid #1473288: You lose unsaved content, if you add/edit/delete a redirect on node edit form I'm just setting 'target' => '_blank' on this link.

This all passes locally, so long as I use run-tests.sh --suppress-deprecations. Otherwise, we get deprecation failures from \Drupal\user\PrivateTempStoreFactory and \Drupal\user\PrivateTempStore. Again, fixing all that is totally out of scope for this issue.

If the d.o testbot is confused/angry about this, perhaps we'll have to tell it to use --suppress-deprecations via a drupalci.yml file or something? I suppose at least that much is in scope for this, if needed. ;) Anyway, let's see what the bot says with this.

Any thing else before this is RTBC?

Thanks!
-Derek

Filed #3025986: Convert automated tests from Simpletest to PHPUnit to convert the existing interface test that extend the deprecated \Drupal\simpletest\WebTestBase

I just stumbled upon this issue. I haven't looked into it, but just wondering if this could be more generalized so that it works for all content entity types, not just nodes?

Re: #22: Thanks!

Re: #23: It could, but that's out of scope for this issue. This is about extending the existing node UI to restore some missing functionality that was present in previous versions. It'd be a much bigger change to move all of that to something general across all content entities. As it is, it's being triggered in redirect_form_node_form_alter() and is totally node-specific. Feel free to open up another follow-up issue if you're interested in generalizing all of it. Somewhat related is #2403891: Interface to remove url redirects on entities. Once again, the divergent features from D7 and D8 in here is a cause for concern. Apparently D7 has already generalized this (too much ;) such that people want checkboxes to hide the redirect UI on (some of) their entity forms), but in D8, it's still specifically tied to nodes.

Minor fix: add the missing doc comment in the test method that the bot complained about.

Sorry, also missed an unused use.

Some nits:

1. +++ b/redirect.module
   @@ -500,5 +501,25 @@ function redirect_form_node_form_alter(&$form, FormStateInterface $form_state, $
   +      'title' => t('Add URL redirect to this %type', ['%type' => $type->label()]),
   

   Possible null pointer here. Maybe we should use the bundle info service here to retrieve the label and add a fallback if the type is not there (which probably never happens.. but just to be sure ;)). Or we could just use '$node->getEntityType()->getLabel();' here instead.

2. +++ b/redirect.module
   @@ -500,5 +501,25 @@ function redirect_form_node_form_alter(&$form, FormStateInterface $form_state, $
   +        'redirect' => "/node/$nid",
   

   I would prefer to use

   $node->toUrl()->getInternalPath()

   instead.

3. +++ b/redirect.module
   @@ -500,5 +501,25 @@ function redirect_form_node_form_alter(&$form, FormStateInterface $form_state, $
   +        'target' => '_blank',
   

   We should have a follow up issue for this as well.

Overall the patch does what it needs to do. At first I missed the language parameter but this is handled separately in the entity form, so it also works for multilingual.

Re: #27: Thanks for the review and testing!

#27.1: Sorry, nope. $node->getEntityType()->getLabel() returns the label for the entity type, in all cases 'Content'. We want the label for the node type (aka bundle). If NodeType::load($node->getType()); returns NULL, we might as well dereference it and catastrophically fail, since your site is so fubar there's nothing this form_alter() is going to do to keep it alive. ;)

#27.2: Actually, that would break this code. This is setting a query parameter to pre-fill the redirect add form at admin/config/search/redirect/add and the To: field there expects the leading slash (like most spots in the UI that are asking for a link/route to a node). If I'm going to have to do a string concatenation to make it work, I might as well generate the string inline instead of 2 chained function calls to get the identical result. Furthermore, earlier in the same function (not touched by this patch), we're already doing this:

    // Find redirects to this node.
    $redirects = \Drupal::service('redirect.repository')
      ->findByDestinationUri(["internal:/node/$nid", "entity:node/$nid"]);

#27.3: You mean the @todo and @see comments immediately preceding this line? ;)

+        // @todo Perhaps this could be more slick and use the off_canvas dialog?
+        // @see https://www.drupal.org/node/2931770
+        'target' => '_blank',

IMHO we don't need a second @see to a possibly duplicate issue on d.o. #1473288: You lose unsaved content, if you add/edit/delete a redirect on node edit form isn't going anywhere fast, and is likely to be solved via #2931770: Improve usability of redirect operation links in node form anyway. *shrug*

Back to needs review for #26.

Cheers,
-Derek

re #27.1: Well makes sense. Even though it has always been named "content" in D7 as well ;)

re #27.2: Not sure if I have something special going on, but without the leading '/' the to field is till correctly pre-filled. Also in d7 the leading '/' was absent. Fine for now though.

Anyways, just some nits to how I would rather like to see it. Works well and goes along with the rest of the code.

RTBC it is ;)!

Thanks for the RTBC!

Re: #27.1: Oh, you mean the D7 version of this feature was boring (per the screenshot). Fair enough. But I think using the node type (e.g. "Article") is more friendly and helpful than keeping it generic. *shrug*.

#27.2: Hrm, yeah, I tried it and you're right, the form seems to add the leading slash itself, after all. *shrug*. I still think #26 is simple and easy, and it's not worth the churn. If/when redirect works with things other than nodes, we can/will revisit this. ;)

Thanks again!
-Derek

+1

Confirming RTBC and looking forward to the commit :) Thank you all!

#26 seems to be working working well enough for me too.

Hi - is there a blocker on getting this committed? If you're coming from a D7 site, you're really used to having this functionality. :)

Confirming that #26 works for me as well.

#26 has been put on a production environment and it works as expected. Thank you!

The patch works fine for nodes, thanks!

#27.1 I suggest using the label "Add URL redirect":
- the context 'this bundle' is implicitly clear
- it's already translated
- demonstrative pronouns can be troublesome to translate. You'd need grammatical variations depending on the context (gender, language, distance to the object) e.g. in Dutch you can say "deze" pagina (this page) but you say "dit" event (this event), in French one says "cette" page but "cet" événement (or évènement), ...

#27.2 Added 'destination' => \Drupal::destination()->get(),

Removed the @todo and opened a new issue to follow this up.

I used the patch in #37 and it works great. I also agree on the grammar issue. It looks clean and to the point now.

@mpp you might want to check the patch naming conventions though.

Thanks to all contributors.

This *was* RTBC. Sadly, no longer. #37 lost the new test coverage I added.

Also, #3061233: Use the off canvas dialog to add url redirects is duplicate with the issue that was already linked in the @todo back in #26.

So, either #26 should be committed, or we need to fix #37 to restore the correct @todo and @see, and to include the tests/src/Functional/RedirectNodeFormTest.php class.

I don't feel *that* strongly about the label. If everyone else thinks it's better to keep it generic and vague, I'm okay with that.

Thanks,
-Derek

p.s. Here's the interdiff between 26 and 37 which is how I noticed we lost all the test coverage and useful code comments.

Whoops, re-added the missing tests.
Having an issue in the queue instead of a @todo should suffice imo.

PS: Can't wait for the move to gitlab so we have proper feature branches (and we no longer need to manually file .patch files and .diff files) o_O.

Tests will fail because I changed the label.

Fixed tests.

Setting back to rtbc.

Please credit dww as author for this issue when committing.

Please credit dww as author for this issue when committing.

respect

@mpp re: #47 -- respect, indeed. ;) Thanks!

Meanwhile, I realized that my original test code (from #21) retained a bogus comment from wherever I copy/pasted some of the test code from. ;) Removed here. Still RTBC.

Thanks,
-Derek

Uhh, wtf? None of those coding standards problems are introduced by this patch. Why is the bot marking this as a test failure?

Weird. The "failure" was at https://www.drupal.org/pift-ci-job/1327695
Requeued and all is well: https://www.drupal.org/pift-ci-job/1327710
Back to RTBC.

Thanks, committed.