Updated: Comment #0
Follow-up from #1987858: Convert taxonomy_autocomplete() to a new style controller and #2056627-22: Form API autocomplete is broken for routes where @tim.plunkett said

If we convert the taxonomy autocomplete before this goes in, then it too will be broken...
If that is the last usage of #autocomplete_path, why not put this in now, and then remove #autocomplete_path over there.

Problem/Motivation

In #2056627: Form API autocomplete is broken for routes we have added route based autocomplete #autocomplete_route. Developers should use #autocomplete_route going forward instead of #autocomplete_path.

Proposed resolution

Remove #autocomplete_path from form_process_autocomplete in form.inc.

Remaining tasks

More discussion and patch.

User interface changes

None.

API changes

New api is added in #2056627: Form API autocomplete is broken for routes we are removing old api.

Files: 
CommentFileSizeAuthor
#8 drupal8.forms-system.2071115-8.patch6.31 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 58,604 pass(es).
[ View ]
#8 interdiff.txt3.24 KBamateescu
#5 drupal8.forms-system.2071115-5.patch6.54 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,447 pass(es).
[ View ]
#5 interdiff.txt2.99 KBjibran
#3 drupal8.forms-system.2071115-3.patch6.53 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 58,176 pass(es), 7 fail(s), and 8,270 exception(s).
[ View ]
#3 interdiff.txt3.04 KBjibran
#1 drupal8.forms-system.2071115-1.patch6.25 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 58,453 pass(es).
[ View ]

Comments

Status:Active» Needs review
Issue tags:+API clean-up
StatusFileSize
new6.25 KB
PASSED: [[SimpleTest]]: [MySQL] 58,453 pass(es).
[ View ]

Let's see how much it fails.

  1. +++ b/core/includes/form.inc
    @@ -4071,17 +4071,13 @@ function theme_vertical_tabs($variables) {
    + * Adds autocomplete functionality to elements with a valid
    + * #autocomplete_route_name.

    Mh, actually the one-line description of a function should be one-line.

  2. +++ b/core/modules/system/system.module
    @@ -325,7 +325,7 @@ function system_element_info() {
         '#size' => 60,
         '#maxlength' => 128,
    -    '#autocomplete_path' => FALSE,
    +    '#autocomplete_route_name' => FALSE,
         '#process' => array('form_process_autocomplete', 'ajax_process_form', 'form_process_pattern'),
         '#pre_render' => array('form_pre_render_textfield'),

    What about also setting #autocomplete_route_parameters to FALSE or an empty array.

StatusFileSize
new3.04 KB
new6.53 KB
FAILED: [[SimpleTest]]: [MySQL] 58,176 pass(es), 7 fail(s), and 8,270 exception(s).
[ View ]

Thank you for the review @dawehner fixed #2.

Status:Needs review» Needs work

The last submitted patch, drupal8.forms-system.2071115-3.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.99 KB
new6.54 KB
PASSED: [[SimpleTest]]: [MySQL] 58,447 pass(es).
[ View ]

Test Fixes

Status:Needs review» Reviewed & tested by the community

Ha, this makes totally sense.

Status:Reviewed & tested by the community» Needs review

Can we not default #autocomplete_route_parameters to an array if #autocomplete_route is set?

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new3.24 KB
new6.31 KB
PASSED: [[SimpleTest]]: [MySQL] 58,604 pass(es).
[ View ]

We do that already, those assignments in system_element_info() were totally not needed.

Here's the relevant part from form_process_autocomplete():

function form_process_autocomplete($element, &$form_state) {
  $access = FALSE;
  if (!empty($element['#autocomplete_route_name'])) {
    $parameters = isset($element['#autocomplete_route_parameters']) ? $element['#autocomplete_route_parameters'] : array();
    $path = \Drupal::urlGenerator()->generate($element['#autocomplete_route_name'], $parameters);
    $access = \Drupal::service('access_manager')->checkNamedRoute($element['#autocomplete_route_name'], $parameters);
  }
...

Fixed that and the doxygen of form_process_autocomplete() which was missing the example path.

Perfect!

Title:Remove #autocomplete_path in favor of #autocomplete_routeChange notice: Remove #autocomplete_path in favor of #autocomplete_route
Priority:Normal» Major
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed/pushed to 8.x, thanks!

Will need a change notice.

Status:Active» Fixed

Title:Change notice: Remove #autocomplete_path in favor of #autocomplete_routeRemove #autocomplete_path in favor of #autocomplete_route
Priority:Major» Normal
Issue tags:-Needs change record+Approved API change

Fixed the change notice title and added issue to change notice. Thank you everybody for the help.

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