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 |
---|---|---|---|
#32 | language-2086499-32.patch | 6.5 KB | tim.plunkett |
#32 | interdiff.txt | 1.52 KB | tim.plunkett |
Comments
Comment #1
phiit CreditAttribution: phiit commentedWorking on this
Comment #2
phiit CreditAttribution: phiit commentedHere goes my first patch, be gentle!
Comment #4
heddnphiit, good work on your first patch. Its close. I noticed a few whitespace issues. Always good to catch these as it makes the committer's life a lot easier. Plus there's a fatal error with the name of the form classes. These need to match what's in the routing.yml file.
There are some whitespace issues with the @inheritdoc comment and the return clause, use only 2 spaces.
The class name needs to match whats in the routing.yml file. Otherwise you get nasty errors:
Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "\drupal\language_elements_test\form\languageconfigurationelementform" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 875 of /home/lucas/websites/drupal/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).
Comment #5
phiit CreditAttribution: phiit commentedAhh, got it. Yeah I wasn't that sure about the class names, I'll give it another go. And somehow I missed a bunch of these whitespaces errors from c&p.
Comment #6
phiit CreditAttribution: phiit commentedFixed class names on routing. Interdiff attached for comparison.
Comment #7
heddn@phiit, this looks good now.
Comment #8
webchickHm. If we're not implementing any code in those methods, I don't think these should be here. Else you're going to end up canceling the upstream validate/submit code that's happening in FormBase, no?
Comment #9
phiit CreditAttribution: phiit commentedI left them there as the class extending FormBase need to implement these methods or they need to be declared abstract. How should I proceed with the new patch?
Comment #10
Gábor HojtsyRight, the problem is formBase is an abstract class that does not even have a submitForm implementation although it says it implements FormInterface. Also validateForm is empty in the abstract class. I guess we can call parent implementations. Not sure if it would blow up if the parent lacks that implementation...
Comment #11
phiit CreditAttribution: phiit commentedContinued working on this issue.
Comment #12
phiit CreditAttribution: phiit commentedFrom the same meta issue:
Issue #1979034: Convert views_test_data_element_form() to a Controller has the same empty methods committed.
Issue #2035689: Convert user_form_test_current_password to a Controller doesn't have validateForm method and it's committed.
I've verified that the methods submitForm and validateForm are indeed abstract. Which way is preferred in this case? a) leave them out or b) use empty methods or c) do something else.
Comment #13
tstoeckler@phiit: What I just told you in person was actually completely wrong as I completely misunderstood this issue.
This can be removed, it's already in FormBase
Re @webchick: FormBase does not actually do any validation and submitting. Also there's a test in core that checks that the language element submit actually is called. So the empty submit should be fine as is.
Comment #14
phiit CreditAttribution: phiit commentedEmpty methods removed.
Comment #15
tstoecklerAhh, sorry for not being precise. The submitForm() actually needs to stay, because that's not in FormBase. Only validateForm() should go. Could you re-roll for that real quick? That would be awesome!
Comment #16
phiit CreditAttribution: phiit commentedComment #17
tstoecklerYes, great. Thanks a lot!
Comment #19
tstoecklerSo this has been sitting for a while and core changed in the meantime:
This should be:
Comment #20
phiit CreditAttribution: phiit commentedAhh alright, changes made.
Comment #21
tstoecklerYay, thanks. Let's see if the bot likes this...
Comment #22
tstoecklerCool, let's do this.
Comment #23
YesCT CreditAttribution: YesCT commented#20: core-language-elements-test-routing-2086499-20.patch queued for re-testing.
Comment #24
catch#20: core-language-elements-test-routing-2086499-20.patch queued for re-testing.
Comment #26
tstoecklerI think this was done in #2102653: Convert test form callbacks to new form controller
Comment #27
tim.plunkettNope, this is still needed.
Comment #29
tim.plunkettNeeded PSR-4 switch.
Comment #30
tim.plunkettAnd removed dead code.
Comment #32
tim.plunkettRemoved the $pid per Jibran's review
Comment #33
jibranSimple enough.
Comment #34
Gábor HojtsyYay, thanks!
Comment #35
alexpottCommitted 5106b55 and pushed to 8.x. Thanks!
Comment #37
Gábor HojtsySuperb, thanks all!