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.
Comment | File | Size | Author |
---|---|---|---|
#48 | 1933518-48.patch | 18.2 KB | pwolanin |
#48 | 1933518-incremental-44-48.txt | 3.84 KB | pwolanin |
#47 | 1933518-46.patch | 18.03 KB | pwolanin |
#47 | 1933518-incremental-44-46.txt | 3.46 KB | pwolanin |
#45 | 1933518-incremental-33-44.txt | 9.97 KB | pwolanin |
Comments
Comment #1
ACF CreditAttribution: ACF commentedComment #3
ACF CreditAttribution: ACF commentedThis should fix the issue. Forgot to explicitly add the submit function and it is needed as other submit functions are added to this form and it was in the old version of the form.
Comment #4
ACF CreditAttribution: ACF commentedFix indentation problems.
Comment #5
ACF CreditAttribution: ACF commentedChanged patch to use the moduleHandler.
Comment #6
amateescu CreditAttribution: amateescu commentedPlease see the review from #1937968-4: Convert statistics's system_config_form() to SystemConfigFormBase which applies here as well.
Comment #7
ACF CreditAttribution: ACF commentedUpdated the patch.
Comment #8
ACF CreditAttribution: ACF commentedUpdated the patch.
Comment #9
Crell CreditAttribution: Crell commentedThis docblock needs to be clearer. The "Verbs X" format says what the method does. It also needs a @return.
Drupal convention is to use protected only, no private methods.
This can be replaced with $this->moduleHandler->loadAllIncludes().
This should be "route" => "search_settings", with no page callback specified.
Comment #10
Crell CreditAttribution: Crell commentedComment #11
ACF CreditAttribution: ACF commentedUpdated with change to the doc block.
Comment #12
ACF CreditAttribution: ACF commentedChanged 'route' => to 'route_name' =>
Comment #13
Crell CreditAttribution: Crell commentedMethods should start with a lowercase character. Sorry for not catching that before.
Otherwise I can't find anything else to object to.
Comment #14
ACF CreditAttribution: ACF commentedComment #16
ACF CreditAttribution: ACF commentedThink this should fix the error.
Comment #18
ACF CreditAttribution: ACF commented#16: 1933518-forminterface-search-drupal-16.patch queued for re-testing.
Comment #20
mtiftApplying just this patch, it fails the BreadcrumbTest (Drupal\system\Tests\Menu\BreadcrumbTest). However, when I apply both this patch and #1872690: Exception: Serialization of 'Closure' is not allowed in serialize, this patch passes the BreadcrumbTest. So it looks as though this patch should be re-tested after 1872690 gets committed.
Comment #21
ACF CreditAttribution: ACF commentedHey thanks for looking at this. I had been looking myself and this patch #1934738: Exception: Serialization of 'Closure' is not allowed in serialize() (line 154 of dblog.module) seems to fix things for me locally as well, so putting this issue on hold until one of these drops.
Comment #22
ACF CreditAttribution: ACF commentedComment #23
ACF CreditAttribution: ACF commented#16: 1933518-forminterface-search-drupal-16.patch queued for re-testing.
Passed tests after #1934738: Exception: Serialization of 'Closure' is not allowed in serialize() (line 154 of dblog.module) was committed.
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedlooks good:) just one thing
should be {@inheritdoc}
Comment #25
ACF CreditAttribution: ACF commentedRe rolled. moved the re-index submit into the searchsettingsform class so I could then remove the system.admin.inc. Think this should be okay.
Comment #27
ACF CreditAttribution: ACF commented#25: 1933518-forminterface-search-drupal-25.patch queued for re-testing.
It seemed like a random fail, it was 1 fail dateintltest fail.
Comment #28
Crell CreditAttribution: Crell commented$this->configFactory isn't defined above It needs to be. Or, actually, you can just grab the one config object we care about here in the constructor and store that as a property.
This can be simplified if we move the ->get() up to the constructor, as above.
Comment #29
mparker17I'll help :)
Comment #30
mparker17Try this...
Comment #31
mparker17So, I made a boo-boo. Member variables should be protected, not private.
Comment #32
mparker17I will fix.
Comment #33
mparker17Try this.
Comment #34
Crell CreditAttribution: Crell commentedI think this covers it for now. Anything else here would be part of a larger search refactor, which is being done elsewhere.
Comment #35
tim.plunkettAll of these $form_state['build_info']['controller'] should just be $this
Comment #36
pwolanin CreditAttribution: pwolanin commentedTurns out the existing tests don't actually test the functionality of this form (apparently it's currently a no-op), so it would be good to see a new test that e.g. fails against current HEAD.
Comment #37
tim.plunkettJust because node's implementation of hook_search_admin has been broken for months isn't a reason to hold up this conversion.
But that point from #35 does need to be fixed.
Comment #38
pwolanin CreditAttribution: pwolanin commented@tim.plunkett - didn't mean to change the status.
Also, just trying to get a handle on this new system - the form object instance is serialized into the form state?
Comment #39
pwolanin CreditAttribution: pwolanin commentedAlso, patch doesn't apply is seemd, maybe due to changes in #1947720: Use Drupal::state() to replace state()
Comment #40
pwolanin CreditAttribution: pwolanin commented@tim - re: #35 - do you really have to declare the submit, etc by hand? I'd have assumed the Form API handles this if the class has an interface requiring those methods?
Comment #41
tim.plunkettOh yeah, I wasn't even paying attention. It does add submitForm for you.
Comment #42
pwolanin CreditAttribution: pwolanin commentedre-roll and cleanup per tim's comments.
Comment #43
pwolanin CreditAttribution: pwolanin commentedok, taking that line out and reformatting a bit
Comment #44
pwolanin CreditAttribution: pwolanin commentedplus doxygen fix.
Comment #45
pwolanin CreditAttribution: pwolanin commentedhere's incremental diff from 33 to 44
Comment #47
pwolanin CreditAttribution: pwolanin commentedComment #48
pwolanin CreditAttribution: pwolanin commentedComment #49
tim.plunkettThat looks great, thanks @ACF and @mparker17 for the initial patch, and @pwolanin for finishing it up!
Comment #50
alexpottCommitted 7b9e229 and pushed to 8.x. Thanks!
Comment #51.0
(not verified) CreditAttribution: commentedUpdated issue summary.