Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UX] Taxonomy term add/edit form: Reevaluate the logic for the path of the "Cancel" link #5207

Open
klonos opened this issue Sep 7, 2021 · 1 comment

Comments

@klonos
Copy link
Member

klonos commented Sep 7, 2021

This is the bit of code that determines the destination of the "Cancel" link in the taxonomy term add/edit form:

  // Prepare cancel link.
  if (isset($_GET['destination'])) {
    $path = $_GET['destination'];
  }
  elseif (isset($_SERVER['HTTP_REFERER'])) {
    $path = $_SERVER['HTTP_REFERER'];
  }
  elseif (user_access('administer taxonomy')) {
    $path = 'admin/structure/taxonomy/' . $vocabulary->machine_name;
  }
  elseif (isset($term->tid)) {
    $path = 'taxonomy/term/' . $term->tid;
  }
  else {
    $path = '<front>';
  }

This code has remained unchanged since 2014, when it was added with #371, but a lot has changed since then, which make it problematic in certain scenarios...

The problem with using multiple elseifs here is that the first elseif that evaluates as TRUE "wins", and the if loop/logic "exits". See: https://www.php.net/manual/en/control-structures.elseif.php

There may be several elseifs within the same if statement. The first elseif expression (if any) that evaluates to true would be executed. ... The elseif statement is only executed if the preceding if expression and any preceding elseif expressions evaluated to false, and the current elseif expression evaluated to true.

The above basically means that the order in which these elseifs are placed in this bit of code determines the "priority" or "weight" each condition gets, should multiple of these conditions be true.

Where that becomes a UX issue is this (mentioned in #999 (comment)):

Clicking "Save and add terms":

  • takes you to the "add term" form for the new vocabulary 👍🏼
  • creating a new term redirects you to a fresh/blank "add term" form, to add more terms 👍🏼
  • clicking the "cancel" link in the "add term" form also redirects you to a blank "add term" form 👎🏼 (you should be redirected to the list of terms for this vocabulary instead)

The fact that $_SERVER['HTTP_REFERER'] is always set, makes it become the "default", which is causing the UX issue described above. Reordering the conditions as such fixes the specific issue:

  // Prepare cancel link.
  if (isset($_GET['destination'])) {
    $path = $_GET['destination'];
  }
  elseif (user_access('administer taxonomy')) {
    $path = 'admin/structure/taxonomy/' . $vocabulary->machine_name;
  }
  elseif (isset($term->tid)) {
    $path = 'taxonomy/term/' . $term->tid;
  }
  elseif (isset($_SERVER['HTTP_REFERER'])) {
    $path = $_SERVER['HTTP_REFERER'];
  }
  else {
    $path = '<front>';
  }

The above variation basically turns $_SERVER['HTTP_REFERER'] and <front> into "fallbacks", while at the same time still respecting $_GET['destination'], which may have been intentionally set (by contrib for example). It is still not "perfect" though, as for starters the user_access() conditional should now be accounting for the more granular permissions that have been introduced with #382. But with UX in mind, I think that the most important workflow logic here should be:

  1. if adding a new term, then "Cancel" should be redirecting you to the list of terms in the respective vocabulary.
  2. if editing an existing term, and you are coming from the list of terms (admin/structure/taxonomy/{VOCABULARY}), then "Cancel" should be redirecting you back to the list of terms in the respective vocabulary.
  3. if editing an existing term, and you are coming from the specific term "view" page (taxonomy/term/{TID}), then "Cancel" should be redirecting you back to the specific term page.
@klonos
Copy link
Member Author

klonos commented Sep 7, 2021

...lets wait for #999 to get in before creating any PR for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant