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.
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 |
---|---|---|---|
#9 | drupal8.search-module.2105609-9.patch | 4.72 KB | RoSk0 |
#9 | interdiff-2105609-7-9.txt | 730 bytes | RoSk0 |
#7 | drupal8.search-module.2105609-7.patch | 4.72 KB | RoSk0 |
#7 | interdiff-2105609-3-7.txt | 2.13 KB | RoSk0 |
#3 | drupal8.search-module.2105609-3.patch | 4.78 KB | RoSk0 |
Comments
Comment #1
vijaycs85Updating tag.
Comment #2
RoSk0Starting...
Comment #3
RoSk0Initial patch version.
Comment #4
jhodgdonThis looks fine to me, and besides which, the tests pass. Thanks!
It's going to conflict with
#1926344: Consolidate search-result.html.twig and search-results.html.twig
Comment #5
alexpottShould be using FormBase's config method
Comment #6
jhodgdonRE #5 - we just need to convert
to
I believe.
RoSk0: If you'd like to make a new patch, with an interdiff, that would be great!
https://drupal.org/documentation/git/interdiff
If you do not have time, unassign the issue and we'll find someone else. Thanks!
Comment #7
RoSk0Update.
Comment #8
jhodgdonThanks!
The interdiff looks right to me.
I also went back and reviewed the whole patch... and I think we should be using the State system instead of the Config system to save the count of how many times the form has been submitted -- it seems like state information and not configuration information. However, I think that is a separate issue. This issue was about converting the existing form to be a form class, and the patch is fine for that. Follow-up: #2122321: search_embedded_form test module is improperly using config instead of state
But there is one small error in this patch:
The docs here say it is for search_embedded_form but the form ID is actually search_embedded_form_form. One or the other should be changed -- they need to match. Thanks!
Comment #9
RoSk0Fixed.
Comment #10
jhodgdonExcellent! I think this is ready to go in now.
Comment #11
jhodgdonWe have two issues that are postponed on this getting committed -- adding them to related issues. Also I'll click retest just in case since it's been a while since the last test.
Comment #12
jhodgdon9: drupal8.search-module.2105609-9.patch queued for re-testing.
Comment #13
Xano9: drupal8.search-module.2105609-9.patch queued for re-testing.
Comment #14
webchickCommitted and pushed to 8.x. Thanks!