Vertical tab for listing redirects to content/entities missing

Yeah, I noticed that, must have gotten lost.

But I'm not exactly sure how that should work now in 8.x.

Seven doesn't use vertical tabs, it converts them to that accordion thing. And there is very, very little space there to show a table of things.

I just wanted to chime in and say that I had a client ask for this specifically.

It's not mission critical, but definitely a UX boost.

I'm more than willing to test patches etc.

Thanks for all of your work on this module! (and others!)

Yeah, the biggest problem is still that I'm not sure where and how to show it? There is no space for it in the sidebar IMHO. A collapsed details element somewhere in the form?

Could it be a simple vertical tab, that just lists the URLs that currently redirect to the node/entity being viewed? and maybe a link to create a new one?

So, it would just list them like:

The following URLs redirect to this node/entity
/node/15
/about-us
/about
/about-our-company
/about-the-team

Add a new Redirect

I don't think it needs to be any more complicated than that. No need for a table or anything. At least not for me.

Maybe it will helpful to look at Metatags, they have their own formatter for the 'Manage form display' page.

Or use a menu tab, so that appears alongside View, Edit, etc.

(hope this is the right tab)

I'd vote for some UX consideration before anything is built.

One thing to keep in mind, is that the data is not specifically attached to the entity, so it doesn't *have* to be on the edit form. However, having it on the edit form gives admins a single place to see all their stuff related to that one entity.

Maybe a vertical tab / accordion section that's kept simple:

Redirects
Current list of redirects for this [entity type label].
Note: changes take effect immediately.
* old/page/alias
* another/old/alias
[add another]

Each existing item could be a link to open an edit form for that item.

Then use an AJAX thing to streamline the add/edit functionality.

Thoughts?

I just realized I suggested the same ui as @bjlewis2, sorry about that X-)

Maybe the form widget could also have a link to the main Redirect admin page for people to see all of the details? I just don't think all of that info is relevant for the edit form.

Just to be clear, when I suggested "sidebar" I meant the right column of the node edit form like what Metatag has.

("words"..)

I should note that Damien's sudden flurry of activity here is because I have been tasked with doing this for a client and asked for opinions. :) I plan to follow Damien's advice so speak quickly if I should be going in a different direction. LOL

FYI I had suggested breaking down the functionality into the following steps:

• Custom field-like thing to show on the entity edit form.
• List of existing redirects for that entity.
• Link to the main redirects admin page that's automatically filtered to the current entity.
• Tiny form for creating a new redirect based on the current path, should just have a "path" and a redirect status selector (301, 302, etc).
• AJAXify the add-new form.
• AJAXify the list of existing paths.

This way it can be built incrementally.

I got a start on this. Unfortunately, it turns out that the form_alter is needed. I added some logic to try and narrow it down to the node edit forms but it adds it to all the tabs so more work is needed there. Could use something better than just throwing a string in as markup, too. But it works for getting the list on there, at least.

Here's a new patch that targets the node edit forms better and uses an item list rather than a br delimited string. It only provides a list at this point, not the link or form, but I figured I'd set it needs review to at least get feedback on this bit. I don't know whether I'm going to be able to work on it more since it fulfills the client's requirements but maybe when I have some contrib time.

Looking at the failed test and most of it doesn't seem to be related to my patch. The only thing that is, is this bit:

fail: [PHP Fatal error] Line 429 of modules/contrib/redirect/redirect.module:
Class 'Drupal\node\Entity\NodeType' not found

And I don't know why that would be since it finds it just fine locally. Anyone know?

1. +++ b/redirect.module
   @@ -418,3 +419,66 @@ function redirect_redirect_operations() {
   +
   +/**
   + * Implements hook_form_alter().
   + */
   +function redirect_form_alter(&$form, FormStateInterface $form_state, $form_id) {
   +  // Get the ids of all node edit forms to target our addition.
   +  $forms = [];
   +  foreach (NodeType::loadMultiple() as $bundle) {
   +    $forms[] = 'node_' . $bundle->id() . '_edit_form';
   +  }
   

   the tests fail because node module is not enabled in some tests.

   There is a better way to do this, even to support any entity type if we want.

   Just for nodes:

   hook_form_node_form_alter().

   That is the base form ID and it is called for any node form.

   If we want to support any entity type, we could do something like flag module:

   function flag_form_alter(&$form, FormStateInterface $form_state, $form_id) {
   $object = $form_state->getFormObject();
   if (!($object instanceof ContentEntityFormInterface) || ($object instanceof ConfirmFormInterface)) {
   return;
   }

   Not sure about putting it on *all* entities though, then we get problems again that it shows up on weird entities where it doesn't really make sense.

   I'm OK with starting with nodes and having a follow-up to think about a sane way to expand it.

2. +++ b/redirect.module
   @@ -418,3 +419,66 @@ function redirect_redirect_operations() {
   +    $path = "internal:/node/$nid";
   

   not sure if this will cover all cases.

3. +++ b/redirect.module
   @@ -418,3 +419,66 @@ function redirect_redirect_operations() {
   + *   Returns an array of paths that are redirected to the given path.
   + */
   +function redirect_get_redirects_to_path($path) {
   +  $query = \Drupal::entityQuery('redirect')
   +    ->condition('redirect_redirect__uri', $path);
   +  $ids = $query->execute();
   

   entity queries work with field names and properties, not colum names. The field name is redirect_redirect and the property is uri, so redirect_redirect.uri.

4. +++ b/redirect.module
   @@ -418,3 +419,66 @@ function redirect_redirect_operations() {
   +  $redirects = [];
   +  foreach ($ids as $id) {
   +    /** @var \Drupal\redirect\Entity\Redirect $redirect */
   +    $redirect = \Drupal::entityTypeManager()
   +      ->getStorage('redirect')
   +      ->load($id);
   +
   +    $redirects[] = $redirect->getSourcePathWithQuery();
   +  }
   

   you should do a load multiple instead. Would also be nice to at least include an edit link in the list?

Thank you very much for the review! I can't remember why I did hook_form_alter() instead of hook_form_node_form_alter(). Very possibly the reason was no more than I bounce between 3 Drupal versions daily and get confused as to what's available. LOL! I'm out of time for the client supporting this for the week but will add this to my to do list next week and get a new patch. Thanks again!

Here's a new patch to address the issues.

#1: Fixed.
#2: I didn't like doing it that way but I honestly have no idea how else to fetch the redirects from the table. Do you?
#3: Fixed. Interesting that it worked fine the way I had it. Is it just smart enough to know what I mean? LOL
#4. Fixed the loadMultple(). I changed the item list to a table and added an "Operations" column. It's just the edit link as text right now because I'm not sure how to do the operations dropdown and am running out of time. So that will need to be in a follow-up patch.

Changes in this patch:
- Renamed "Current redirects" to "URL redirects" (to match D7).
- Switched to hook_form_node_form_alter() to target the right forms.
- Changed to new style array syntax.
- Corrected syntax in entity query.
- Switched to using loadMultiple().
- Switched the output to a table that has an edit link.

Here's what it looks like, now:

PS: I attached a diff between the two versions of the patch and it's trying to test it. How do you make it not do that?

Quick review, looks pretty good already visually. What happens if you have longer redirect source strings? Usually they're more like /some/hierarchy/2017-04-12/pretty-long-node-title which might be a problem with wrapping..

interdiff: name it -interdiff.txt or do-not-test.patch to not have it tested.

1. +++ b/redirect.module
   @@ -418,3 +419,63 @@ function redirect_redirect_operations() {
   +  $header = ['Redirect', 'Operations'];
   

   header should use t() too.

2. +++ b/redirect.module
   @@ -418,3 +419,63 @@ function redirect_redirect_operations() {
   +    'markup' => [
   

   markup as a key but then it is a table is a bit strange, I'd use 'table' as the key here.

3. +++ b/redirect.module
   @@ -418,3 +419,63 @@ function redirect_redirect_operations() {
   +function redirect_get_redirects_to_path($path) {
   

   this sounds like an API function, but it's specifically built for your table structure. I would just inline this into the other function and use $rows instead of $redirect_paths, which is the common variable name when building table rows.

   Instead of using #rows, it would also be possible to use render array children, then you can use render arrays. That means you could use toLink()->toRenderable(). the path needs to use ['#plain_text' => $path] then.

4. +++ b/redirect.module
   @@ -418,3 +419,63 @@ function redirect_redirect_operations() {
   +    $edit_link = $redirect->toLink('(edit)', 'edit-form')->toString();
   

   edit also needs to be translatable, not sure about the (), I get why you added them but would just do t('edit'), might also be confusing for translators otherwise.

   not sure if a operations dropdown would work well here, I'm fine with just the edit link.

One more thing, we need to include an access check to make sure that this is only visible to users who are allowed to manage redirects.

Thanks for the quick review! I unfortunately ran out of time again but will pick this back up ASAP and get the updates. Thanks for the interdiff tip. :)

I applied the latest patch against 8.x-1.x-dev, but only the labels appeared in the edit form...there were no fields.

+++ b/redirect.module
@@ -418,3 +419,63 @@ function redirect_redirect_operations() {
+function redirect_get_redirects_to_path($path) {
+  // Get a list of redirects for the given path.
+  $query = \Drupal::entityQuery('redirect')
+    ->condition('redirect_redirect.uri', $path);
+  $ids = $query->execute();
...
+  // Load all the redirect entities.
+  $redirects = \Drupal::entityTypeManager()
+    ->getStorage('redirect')
+    ->loadMultiple($ids);

I think this part should be moved as a method in the \Drupal\redirect\RedirectRepository service as a new ::findByDestination() method. This would allow, for example, to get all redirects of an entity:

$redirects = \Drupal::service('redirect.repository')->findByDestination('entity:node/123');

I implemented #28 in a separate ticket #2884587: Find all redirects given a destination URI.

Finally had a chance to work on this thanks to the #tcdrupal sprint. :)

From #24, 1, 2, and 4 fixed. For #3, I changed the variable name but didn't really understand this:

Instead of using #rows, it would also be possible to use render array children, then you can use render arrays. That means you could use toLink()->toRenderable(). the path needs to use ['#plain_text' => $path] then.

You are right that long URLs are a problem. Unfortunately, I don't know how to fix it. Any ideas?

From #25, added the permission check.

From #28, made use of the new function.

I'm leaving this as needs work both because of the above issues and because it is now dependent on #2884587: Find all redirects given a destination URI

It's nice to have this functionality as a view and in a tab, thanks for the patches.

The way we put it in the tab though could still be improved by making it available as a field on the entity form display. This way the order can be managed on "Manage form display". If this is not an option, I wouldn't inject it in a separate tab but in the existing "URL alias" tab.

#24 To fix the long urls, you can use CSS ellipsis to "cut" the label and the title attribute to provide the full path in a tooltip:

<div title="/a-very-long-path" style="white-space:nowrap; overflow: ellipsis; overflow: hidden; text-overflow: ellipsis; width:50px;">/a-very-long-path</div>

+    // Get a list of paths that redirect to this node.
+    $redirects = \Drupal::service('redirect.repository')
+      ->findByDestinationUri("internal:/node/$nid");;

This is a question, not a suggestion: Do we need to also get the aliases of the node and find the redirects to those? Or does redirect.repository already do that?

#27 Added empty text to the table to indicate no redirects are available.

#28/29 Per 2884587-4 I moved the \Drupal::service('redirect.repository')->findByDestination method back to this issue.

#30 Implemented render array children for $rows

#32 Implemented hook_entity_extra_field_info() similar to the D7 version. The weight => 50 places URL redirects directly below the URL path settings for a Standard installation.

#33 Implemented text-overflow: ellipsis; so long redirect paths are truncated, but kept the full path available on the title attribute so it is displayed on hover.

#34 The current implementation lists all redirects to the node URI (internal:/node/[nid] as well as entity:node/[nid]) but not aliases of the current node, similar to the D7 version.

Implemented the following additional changes:

1. Table headers updated to 'From', 'Operations' similar to the D7 version
2. Added redirect entity operations
3. Redirects tab is not added to the node form when adding a new node, similar to D7 version.

Screencap of #35:

Let's not assume the node module is installed, as this causes the redirect domain test to fail.

1. +++ b/redirect.module
   @@ -401,3 +401,91 @@ function redirect_redirect_operations() {
   +  if ($entity_type_manager->getDefinition('node_type', FALSE)) {
   

   You can use hasDefinition() instead. Could also be a check on node, since we're adding to node in the end. Or a module exists check on the node module.

2. +++ b/src/RedirectRepository.php
   @@ -134,6 +134,23 @@ class RedirectRepository {
   +   * @return \Drupal\redirect\Entity\Redirect[]
   +   *   Array of redirect entities.
   +   */
   +  public function findByDestinationUri($destination_uri) {
   

   what about accepting an array of destination URI's here, our main use case for this does need exactly that and it just requires two characters more if you don't have more than one. The we can do an IN query.

#38.1 Added check on node module instead of checking for the entity type definition

#38.2 RedirectRepository::findByDestinationUri now takes an array of strings instead of a string and updated redirect_form_node_form_alter() accordingly

Thanks, maybe also update the comment on the changed @param to say that it is "A list of destination.. " or so?

#40.1 Updated @param description to 'List of destination URIs, for example ['internal:/node/123']'

Thanks. Tested this manually, works quite well.

One thing I noticed is that there is currently no destination, so after deleting, you end back on the overview, but that will be fixed automatically in 8.5.x. The more problematic part is that it is quite easy to lose data, because clicking on those operations will lose all data that has been customized on the form. Not sure what to do about that, I guess it was the same in 7.x. Maybe there could be a title on the links or a text above or below the table?

Also, we're going to need some tests, this is a non-trivial feature addition. Should not be too complicated. \Drupal\redirect\Tests\RedirectUITest::testAutomaticRedirects() uses the node form to test automatic redirections. All we need to do is add a few asserts. First make sure that initially, the url redirects UI is not shown, after the first rename, make sure the old alias shows up there and so on. Possibly also check a delete link at the end. Asserting the resulting page is a bit tricky since it will vary between 8.5.x and 8.4.x.

#42.1 Added a warning below the redirect table 'Note: links open in the current window.' so users expect to leave the node form. I have created a followup issue to (further) improve the usability: #2931770: Improve usability of redirect operation links in node form

#42.2 Added assertions for the empty text in the redirect table and operation links for Edit/Delete. RedirectUITest::assertLinkByHref checks for partial matches, so these should be okay in both 8.4.x and 8.5.x. I have not included assertions for interactions on the Redirect edit/delete pages, since these are out of scope of the current addition.

Let's see if this fixes the failing tests.

Thanks, that works for me for a first version.