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.
* Inject all the things
* Use FormBase and ControllerBase for $this-t(), etc.
* Use a proper library instead of attaching the CSS manually
* ...
Comment | File | Size | Author |
---|---|---|---|
#16 | interdiff.txt | 2.39 KB | Gábor Hojtsy |
#15 | 2110491-15.patch | 30.78 KB | tstoeckler |
#15 | 2110491-13-15-interdiff.txt | 1.07 KB | tstoeckler |
#13 | 2110491-13.patch | 30.58 KB | tstoeckler |
#13 | 2110491-11-13-interdiff.txt | 2.73 KB | tstoeckler |
Comments
Comment #1
tstoecklerComment #2
tstoecklerHere we go.
Comment #4
tstoecklerSome comments on the changes.
The only real change here is ConfigNamesMapper -> ConfigMapperInterface. I reformatted the rest to be alphabetically, as that's what PhpStorm does automatically.
The alternative to calling \Drupal::getContainer() would be injecting all the services needed by the forms into the parent controller. But once we can finally remove our hook_menu() implementation, the forms will be on separate routes anyway, and then they will need the create() anyway, so I think this is the most future-proof way to do it.
See above.
See above. Also ModuleHandler -> ModuleHandlerInterface and FormInterface -> FormBase.
There are two changes here:
1. Putting all of the configurations into a separate form key. I think this is best practice, since we're adding other top-level form keys below.
2. Instead of keying by $id, which is just a random array key, I think it makes sense to key by something meaningful. That will make altering so much easier.
Looking at this now, this will not work, because I did not set #tree on $form['config_names'];
See above.
I realized this when testing the another issue. Thinking about this some more, I think it would make even more sense to use a validate() method to prevent cases 2. and 3.
I think this makes the code much easier to read. It removes the class_exists() but that's just babysitting broken code IMO.
This is sort of my personal preference, but I think it's very uncommon to have full docblocks in inline code, so I think this will find more agreement in general.
Comment #5
tstoecklerThis fixes some tests and #4.6
Comment #7
tstoecklerRunning tests takes a while on my machine, so let's just try this one in the meantime.
Comment #9
tstoecklerOops, wrong patch.
Comment #11
tstoecklerWell, that was dumb. Still haven't run this locally :-/
Comment #13
tstoecklerAll right, fixed the remaining fails. One was due to #1958754: Copy improvements to default Views so basically all patches should currently fail for that. See the $description change in the interdiff.
Comment #14
tstoecklerYay, RTBC anyone? :-)
Comment #15
tstoecklerAhh, the $typed_config_manager is missing from the docs.
Quick re-roll for that.
Comment #16
Gábor HojtsyYay, thanks! I committed this with the minor changes attached.
1. Added missing $this->t conversions.
2. Removed a @todo about the different messages you wanted to add. I don't agree that we'd need different messages. If the translations happen to be exactly the same as the sources, I think it makes sense for us to remove the file for performance at least. We could also keep an empty file if we are concerned for a tool that would discover "untranslated" things would get the wrong impression. But how we store when translations are identical to sources is not really the business of the translator IMHO. I don't think we should inform them that now we *deleted* translations, because what happened is the translation is the same as the source.
3. Edited an existing TODO, since that issue seems to not be getting to core with some seemingly good reasons :)
It would be great to continue with other modernizations, eg. #2104925: Convert to work with routes instead of paths in mappers or more test coverage at #2095383: Improve config translation test coverage by adding a test module :)
Comment #17
Gábor HojtsyOr figuring out the AJAX callback and test coverage at #2099997: Use config translation to translate date formats which are missing for that to move :)
++ :)
Comment #18
tstoecklerWell at the very least we should display a different message when a translation is *added* (vs. updated), anything else is just weird.
I still think we should do some validation on an "empty" translation, but I'm fine with being overruled on that detail.
In general I'm still working on the automatic config form discovery, I just came across some of this there so I spun it off.
Thanks for the quick commit.
Comment #19
Gábor HojtsyYeah adding vs. updating sounds very logical to have separate messages yup :)
Comment #20
Gábor HojtsyThat is now being handled in #2111087-9: Make the translation add and edit forms separate classes onwards.