Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Updated: Comment #18
Problem/Motivation
Converting search_view to Controller/Form.
Proposed resolution
- search_view menu entry converted to Controller extending ControllerBase and injecting plugin.search.manager
- search_form menu entry converted to Controller extending FormBase and injecting plugin.search.manager
- Use YAML discovery for local tasks #2050919: Replace local task plugin discovery with YamlDiscovery
Remaining tasks
Use YAML discovery for local tasks (needs built on #2050919: Replace local task plugin discovery with YamlDiscovery)
Related Issues
#2003482: Convert hook_search_info to plugin system
#2050919: Replace local task plugin discovery with YamlDiscovery
Original report by @vijaycs85
Part of #1971384: [META] Convert page callbacks to controllers
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Comment | File | Size | Author |
---|---|---|---|
#53 | search-1987806-53.patch | 17.24 KB | tim.plunkett |
#53 | interdiff.txt | 2.21 KB | tim.plunkett |
#51 | 1987806-search-controller-51.patch | 16.91 KB | kim.pepper |
#51 | interdiff.txt | 881 bytes | kim.pepper |
#50 | 1987806-search-controller-50.patch | 16.91 KB | kim.pepper |
Comments
Comment #1
kim.pepperInitial attempt.
Comment #2
kim.pepperFixed up docs, and removed ControllerInterface since we aren't injecting anything.
Comment #4
kim.pepperUse _content instead of _controller, try fixing routing, and clean up docs.
Comment #6
kim.pepperI needed to include the search.pages.inc to get the form validate and submit functions to work.
Also cleaned up some code style issues.
Comment #8
dawehnerThis is looking pretty great!
Ha, one empty line too much :)
Maybe just remove "Page callback"...
Just wondering: What does "~" stand for?
Comment #9
kim.peppermodule_handler
Comment #10
kim.pepper@dawehner ~ is NULL in yaml
Comment #12
kim.pepperI'm postponing until #2003482: Convert hook_search_info to plugin system is in as it touches the issues with routes.
Comment #13
kim.pepperUn-postponing
Comment #14
jhodgdonkim.pepper: do you still plan to do this one? If not, perhaps it should be assigned to pwolanin.
Comment #15
kim.pepperjhodgdon Yes, I've started on this. I'll post something shortly.
Comment #16
kim.pepperOK, this is not working at all, but I though I should post it so pwolanin could help out.
It's a lot more work that I originally thought. It requires:
- a route controller
- a form interface
- 2 different access checks
- a route subscriber
I'm some way through this, but hit a few issues. Happy for anyone else to carry it forward.
Kim
Comment #17.0
(not verified) CreditAttribution: commentedAdded link to plugin issue
Comment #18
disasm CreditAttribution: disasm commentedCreating SearchForm Class, injecting plugin.search.manager. Changing SearchController to use new form, removing search_form.
Comment #19
tim.plunkettThis should use StaticAccessCheckInterface
This should just be
return $this->searchPluginManager->getActiveDefinitions() ? static::ALLOW : static::DENY;
and the route should
_permission: 'search content'
and_access_mode: 'ALL'
These need some help
Use $this->urlGenerator()
Extra blank line
You don't need to implement this
Some weird extra blank lines
This should be wrapped onto multiple lins
Why aren't these ported?
Hm, what's all this?
This should match
Comment #21
disasm CreditAttribution: disasm commentedRegarding #10, SearchInterface has a method that allows a search plugin to alter the form before it's returned to the user. See line 85 of SearchInterface.php.
Addressing remaining comments in #19.
Comment #23
kim.pepper#21: drupal8.search-module.1987806-21.patch queued for re-testing.
Comment #24
kim.pepperComment #26
disasm CreditAttribution: disasm commentedI'm not sold on the access checks the way they are, with the one extending the other. At any case, this is going to be dependent on the current user service being accessible in access checks because the SearchPluginManager::pluginAccess takes $account as a parameter.
Comment #27
dawehnerLet's better return static::ALLOW and static::DENY.
Comment #29
disasm CreditAttribution: disasm commentedfixing return value on access checker
Comment #31
xjmThanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.
Comment #32
disasm CreditAttribution: disasm commentedreroll and test failure fixes.
Comment #34
jhodgdonThose failures appear to be real (in search tests).
Comment #35
vijaycs85Fixing test cases(Locally green!!!).
Credit: Thanks to @tim.plunkett and @dawehner for the support on IRC to fix test failures.
Comment #36
dawehnerThis method seem to lack a one line description.
There are newlines missing here.
I guess a little bit more documentation could make it clearer what is going on.
Someone could open a follow up to add support for routes on form redirect.
Comment #37
vijaycs85thanks for the quick review @dawehner. here is the update on individual item with revised patch.
#36.1 - Fixed
#36.2 - Fixed
#36.3 - Fixed
#36.4 - Created new follow up at #2105657: Add support for routes in form redirect.
Comment #38
tim.plunkettOpened #2105661: Add support for $form_state['redirect_route']
Comment #39
dawehnerThank you!
Comment #40
jhodgdonThe documentation on this patch needs some work.
a) The controller's view() method is documented as:
This is not good English and it is not accurate. How about:
Creates a render array for the search page.
b) The 2nd and 3rd parms for this same method:
In $plugin_id, it is not accurate to say "i.e. the data type". Just say "The ID of a search plugin (and note that it is ID not id).
In $keys, this is not following standards. It needs to start with "S" not "s".
c) In SearchForm class:
There is an extra line in the doc block. And if you are going to fix that, you should also change "a" to "the" in the first line.
Comment #41
kim.pepperDocs fixes for #40
Comment #42
jibranAs #40 is fixed so back to RTBC.
Comment #43
jhodgdonThank you!
Comment #44
alexpottNeeds a reroll
Comment #45
kim.pepperRe-roll
Comment #46
jhodgdonI verified this is the same patch (one context line in routing file changed; everything else the same)... Assuming test bot agrees, this should be back to RTBC.
Comment #47
Xano#45: 1987806-search-controller-45.patch queued for re-testing.
Comment #48
pwolanin CreditAttribution: pwolanin commented#45: 1987806-search-controller-45.patch queued for re-testing.
Comment #49
alexpottNot needed
This should be changed to
drupal_get_form('\Drupal\search\Form\SearchForm', $plugin);
and then you won't need the use statement either.Comment #50
kim.pepperFixes for #49 plus make use of the new formBuilder interface.
Comment #51
kim.pepperDamn. Wrong method. Should be getForm() not buildForm().
Comment #52
jhodgdonThanks! That looks like the correct fix for #49 to me. Back to RTBC.
Comment #53
tim.plunkettNot quite there yet.
Comment #54
kim.pepperThanks Tim. Much better. Forgot we were already using ContainerInjectionInterface. Back to RTBC.
Comment #55
alexpottCommitted 8546c8f and pushed to 8.x. Thanks!
Comment #55.0
alexpottApplying template