To reproduce:

- Go to admin/structure/block/list/block_plugin_ui%3Abartik/add
- Fill in a search word in and hit 'Next'

You get a notice and are also (second bug?) redirected to the wrong page.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Title: Notice: Undefined index: theme in Drupal\block\Plugin\system\plugin_ui\BlockPluginUI->formSubmit() » Autocomplete broken to search blocks - also triggers notice after clicking 'Next'
swentel’s picture

Status: Active » Needs review
FileSize
1.7 KB

Brings back the autocomplete (code by EclipseGc) and fixes the redirect as well.

swentel’s picture

Just a question: should we validate the machine name on submit, because otherwise, you get a big error message like:

"Drupal\Component\Plugin\Exception\PluginException: The plugin (recent_comments_) did not specify an instance class. in Drupal\Component\Plugin\Factory\DefaultFactory->getPluginClass() (line 60 of /Users/drupal/drupal/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php)"

aspilicious’s picture

the machine name has an issue alrdy to turn it into an autocompleted field (like we have with other machine names). We should add more validation there. (and add some tests)

aspilicious’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.moduleundefined
@@ -1122,6 +1122,9 @@ function system_plugin_ui_form($form, &$form_state, $plugin, $facet = NULL) {
 function system_plugin_autocomplete($plugin_id, $string = '') {
+  $string_typed = drupal_container()->get('request')->query->get('q');
+  $string_typed = drupal_explode_tags($string_typed);
+  $string = drupal_strtolower(array_pop($string_typed));

Whats the point of a string argument if it gets overriden anyway?

EclipseGc’s picture

In the code I sent to swentel the first time I had removed that, he probably just missed it when he added it to his code.

I would like a validation on this autocomplete to make sure that we're requesting a valid plugin_id. I think that makes a lot of good sense.

EclipseGc’s picture

Status: Needs work » Needs review
FileSize
2.41 KB

ok, this fixes the issue aspilicious pointed out, and adds some validation to the form to prevent the issues swentel mentioned.

Eclipse

tim.plunkett’s picture

Issue tags: +Needs tests

Tagging

swentel’s picture

Assigned: Unassigned » swentel

Will add tests

swentel’s picture

Issue tags: -Needs tests
FileSize
4.27 KB
1.86 KB

Tests only + Tests and fix patch.

Status: Needs review » Needs work

The last submitted patch, 1880378-10.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#10: 1880378-10.patch queued for re-testing.

tim.plunkett’s picture

The fix looks good, but a couple nitpicks.

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockLibrarySearchTest.phpundefined
@@ -0,0 +1,61 @@
+  protected $regions;

Missing a docblock

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockLibrarySearchTest.phpundefined
@@ -0,0 +1,61 @@
+  function setUp() {
...
+  function testBlockLibrarySearch() {

Should be protected.

+++ b/core/modules/system/system.moduleundefined
@@ -1121,7 +1121,10 @@ function system_plugin_ui_form($form, &$form_state, $plugin, $facet = NULL) {
  * @todo This needs more explanation and parameter documentation.
  */
-function system_plugin_autocomplete($plugin_id, $string = '') {
+function system_plugin_autocomplete($plugin_id) {

This seems like a good time to fix this todo.

EclipseGc’s picture

FileSize
4.73 KB

Ok, attempted to take care of tim's comments. setUp() is now protected, we discussed the test method and agreed that didn't need to be protected. The variable that he wanted docs for was completely unnecessary. I removed it. More extensive docs added for system_plugin_autocomplete().

Eclipse

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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