Hello,

In https://cgit.drupalcode.org/bootstrap/tree/src/Plugin/ProcessManager.php..., the findButton() method does not find the nearest button as in the method PHPDoc, but the last one, as there is no break in the foreach when finding a button.

I found the problem in a form with multiple submit buttons.

I will provide a patch in a few hours and also testing on the last dev version of the theme.

Comments

Grimreaper created an issue. See original summary.

grimreaper’s picture

Version: 8.x-3.7 » 8.x-3.x-dev
Assigned: grimreaper » Unassigned
Status: Active » Needs review
StatusFileSize
new869 bytes
new3.05 KB
new3.86 KB
new3.04 KB

Tested on the last dev version.

There is still the bug.

I have uploaded a fix.

I have done tests modifying directly the native search block form to add submit button. www/core/modules/search/src/Form/SearchBlockForm.php:

    $form['keys'] = [
      '#type' => 'search',
      '#title' => $this->t('Search'),
      '#title_display' => 'invisible',
      '#size' => 15,
      '#default_value' => '',
      '#attributes' => ['title' => $this->t('Enter the terms you wish to search for.')],
    ];
    $form['keys']['test1'] = [
      '#type' => 'submit',
      '#value' => $this->t('test 1'),
    ];

    $form['actions'] = ['#type' => 'actions'];
    $form['actions']['test 2'] = [
      '#type' => 'submit',
      '#value' => $this->t('test 2'),
    ];
    $form['actions']['submit'] = [
      '#type' => 'submit',
      '#value' => $this->t('Search'),
      // Prevent op from showing up in the query string.
      '#name' => '',
    ];

bootstrap_button_1.png: the result without the patch.
bootstrap_button_2.png: the result with the patch.
bootstrap_button_3.png: the result with the patch and with the "test 1" button commented out.

Thanks for the review.

markhalliwell’s picture

Status: Needs review » Needs work

Interesting. Cannot believe I missed that heh

The findButton method really doesn't belong in ProcessManager anyway and should instead live in the Element class instead.

I think this patch should move the method (getting rid of the unnecessary $element parameter) and then deprecate this existing method?

Then it should be a simple as $button = &$parent->findButton() in processInputGroups

grimreaper’s picture

Status: Needs work » Needs review
StatusFileSize
new2.6 KB

Thanks @markcarver for the review.

Here is a new patch with your suggestion.

About the deprecation, I don't think it will be very frequent to have custom theme using the findButton method of the processManager. And maybe putting a warning in the next release note and maybe a change record will be good.

markhalliwell’s picture

Status: Needs review » Needs work

Don’t remove the method in ProcessManager, just deprecate it and return $element->findButton();.

Also, above that, call Bootstrap::deprecated().

grimreaper’s picture

Status: Needs work » Needs review
StatusFileSize
new2.61 KB

OK... Now I see how you handle deprecation.

Here is a new patch.

Thanks for the review.

  • markcarver committed 7d58a10 on 8.x-3.x authored by Grimreaper
    Issue #2944760 by Grimreaper: Form preprocess not finding the right...
markhalliwell’s picture

Status: Needs review » Fixed
grimreaper’s picture

Thanks for the commit :)

Status: Fixed » Closed (fixed)

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