Follow-up from #1971384: [META] Convert page callbacks to controllers.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | language_browser_form-2082071-5.patch | 13.53 KB | plopesc |
| #5 | interdiff.txt | 2.65 KB | plopesc |
| #1 | language_browser_form-2082071-1.patch | 13.31 KB | plopesc |
Follow-up from #1971384: [META] Convert page callbacks to controllers.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | language_browser_form-2082071-5.patch | 13.53 KB | plopesc |
| #5 | interdiff.txt | 2.65 KB | plopesc |
| #1 | language_browser_form-2082071-1.patch | 13.31 KB | plopesc |
Comments
Comment #1
plopescHello, attaching patch for this issue.
There are a couple of issues related to
language_get_browser_drupal_langcode_mappings()andlanguage_set_browser_drupal_langcode_mappings()functions, used in the current form implementation.Given that these functions assume that we are calling it from procedural code, they call to
Drupal::config(). I replaced it in the new form class by calls to$this->configFactory()using dependency injection. Then I found thatNegotiationBrowserDeleteFormclass uses both functions, I don't know if you preferred this approach in these cases.IMHO, we should use the dependency injection in form classes. What do you think?
Regards
Comment #2
disasm commentedreplace with parent::__construct($config_factory, $context);
As for dependency injection, your approach is better than the way it was done in NegotiationBrowserDeleteForm. Don't fix that here though, that can be done in a separate issue.
Comment #3
disasm commentedComment #4
dawehnerLet's add title to the defaults->_title route definition.
We should use the ModuleHandlerInterface instead of just the ModuleHandler.
Missing documentation.
Comment #5
plopescHello
Attaching patch including suggestions in #2 and #4.
@dawehner: I added
_titlein routing file. However, I didn't remove it in hook_menu, because in that case, this page doesn't show the title. Am I missing something?Regards.
Comment #6
plopesc@dawehner: Maybe
HtmlFormControllerdoes not take into account_titleproperty?HtmlPageControllercode tries to find it, butHtmlFormControllerjust returndrupal_build_form. I was not able to find where_titleis processed in this case.Regards.
Comment #7
dawehnerMh right, I know that aspilicious was working on a fix, so let's keep it like that.
Comment #8
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 #9
disasm commentedmoving back to rtbc, xjm cross posted.
Comment #10
gábor hojtsyComment #11
webchickCommitted and pushed to 8.x. Thanks!