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.
This issue covers below changes:
- removed helper language_entity_supported
- Convert content_language_settings_page to new controller
- Convert content_language_settings_form to new formController
Blocked/Postponed on this:
#933004: Test that all form elements have a title for accessibility
Follow-up (postponed on this):
Being done here, issues marked duplicate of this one:
#2138151: ContentLanguageSettingsForm is unused (and broken)
Might need a re-roll when this goes in
#2115427: Convert language_content_settings_form to FormInterface
Comment | File | Size | Author |
---|---|---|---|
#133 | 1987726-language-controller-133.patch | 11.08 KB | Gábor Hojtsy |
#127 | 1987726-language-controller-127.patch | 11.1 KB | vijaycs85 |
Comments
Comment #1
pdrake CreditAttribution: pdrake commentedComment #2
pdrake CreditAttribution: pdrake commentedComment #4
pdrake CreditAttribution: pdrake commentedComment #5
dawehnerManual testing the patch works pretty good.
It's odd to actually move an api function into a protected method on the controller, even there is just a single user of it.
This could use {@inheritdoc}
Comment #6
pdrake CreditAttribution: pdrake commentedI moved the API function because this is the only use in core and I could not locate any uses in contrib for it. I could certainly make it a public function or simply put it back where it was, but it seemed to only serve a function within the content language settings form.
Comment #7
dawehnerWell, it feels really helpful. Thanks for searching in contrib, but afaik the language module got introduced in D8 so there is a low chance that actually these functions are used already.
I asked the i18n people for their opinion.
Comment #8
pdrake CreditAttribution: pdrake commentedComment #9
pdrake CreditAttribution: pdrake commented@dawehner, thanks for inquiring with i18n folks. Are there any outstanding issues with the patch besides making a final determination on that function?
Comment #10
dawehnerAs far as I understood gabor he said that people which deal with kind of problems need other information from the entity info anyway so they don't really loose anything by not heaving this function.
Needs an @return
This should be better an entry in hook_library.
Comment #11
pdrake CreditAttribution: pdrake commentedComment #13
vijaycs85Comment #15
vijaycs85Fixing test case...
Comment #17
vijaycs85#15: 1987726-convert_language_content_settings_to_controller-15.patch queued for re-testing.
Comment #19
stella CreditAttribution: stella commented#15: 1987726-convert_language_content_settings_to_controller-15.patch queued for re-testing.
Comment #21
stella CreditAttribution: stella commentedPatch reroll
Comment #23
stella CreditAttribution: stella commented#21: 1987726-convert_language_content_settings_to_controller-21.patch queued for re-testing.
Comment #25
dawehnerThat is just fixing some other unrelated issues.
Comment #27
vijaycs85#25: drupal-1987726-25.patch queued for re-testing.
Comment #28
vijaycs85Comment #30
dawehnerComment #32
vijaycs85Re-roll with syntax error fix.
Comment #33
vijaycs85Adding the file that is missing in #32.
Comment #35
disasm CreditAttribution: disasm commentedreroll
Comment #37
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 #38
Gábor HojtsyIn short, we have 3 more days to get this to RTBC or we'll need to split this to two issues and do it in 2 steps. Anybody wanna jump on this? :)
Comment #39
disasm CreditAttribution: disasm commentedInterestingly, this form is already in core, just not being used, and it's slightly broken. Fixing the form, removing the the form callbacks. I'm not sure splitting this one is going to help any. All the test failures are a result of hook_element_info not processing things correct. The code in the form is nearly identical (with a performance improvement, but I verified same behavior without it).
Here's the errors for anyone that wants to take a look:
Comment #41
disasm CreditAttribution: disasm commentedstill haven't fixed index error. This resolves the VERSION error though by removing the unused library (single css makes more sense as a css attached anyways).
Comment #43
disasm CreditAttribution: disasm commentedReroll. Using drupalPostForm instead of drupalPost.
Comment #45
Gábor HojtsyHah, how is the ViewAjaxControllerTest.php change related?
Comment #46
disasm CreditAttribution: disasm commentedSave -> Save Configuration: The SettingsFormBase uses Save Configuration as the text on the Submit button.
Comment #47
andypostLet's try this.
I think no reason to change button text in conversion.
Comment #48
kgoel CreditAttribution: kgoel commentedUn-assigning this issue. I will review it.
Comment #49
kgoel CreditAttribution: kgoel commented[Dupe]
Comment #50
kgoel CreditAttribution: kgoel commentedI had re-roll #47 patch because there was three conflicts in the head. First conflict was in language.module where router had different name 'route_name' => 'language.content_settings_page' so I left this route name. Second conflict was in language.routing.yml had title under default, and third conflict was in language.admin.inc which had just this one line (@deprecated Use \Drupal\language\Controller\LanguageController::contentSettings()) added for @deprecated Use \Drupal\language\Controller\LanguageController::contentSettings().
Comment #51
andypostyep, we have content translation totally broken in core #1946462-52: Convert content_translation_translatable_form() to the new form interface
Comment #53
kgoel CreditAttribution: kgoel commentedI have combined https://drupal.org/node/1946462#comment-7876287 and https://drupal.org/node/1987726#comment-7883229 patch to see if https://drupal.org/node/1946462#comment-7876287 fixes this.
Comment #55
googletorp CreditAttribution: googletorp commented#53: drupal8.language-module-1987726-53.patch queued for re-testing.
Comment #57
disasm CreditAttribution: disasm commentedFixes test failures. The temporary controller and route info needed removed.
Comment #58
dawehnerNice, we actually add documentation.
Comment #59
Xano#57: drupal8.language-module.1987726-57.patch queued for re-testing.
Comment #60
alexpottPatch no longer applies.
Comment #61
XanoComment #62
Gábor HojtsyComment #63
alexpottEither we have scope creep here or the issue summary needs work because this patch is definitely not just doing what it says "Convert language_content_settings_page() to a new style controller"
Can we either have an issue summary and title update along with some explanation or we can separate out some of the changes introduced by #53.
Comment #64
andypostSeems better to postpone the issue on the same reason as #1946462-59: Convert content_translation_translatable_form() to the new form interface
Comment #65
vijaycs85Seems we need to combine them to make work. Updating title and issue summary with scope of the patch.
Comment #66
vijaycs85#61: drupal_1987726_61.patch queued for re-testing.
Comment #66.0
vijaycs85Updated issue summary.
Comment #67
Gábor HojtsyPatch fits the current scope. :)
Comment #68
vijaycs85Restoring tags and +1 for RTBC.
Comment #69
Xano61: drupal_1987726_61.patch queued for re-testing.
Comment #70
Xano61: drupal_1987726_61.patch queued for re-testing.
Comment #71
tim.plunkettThis seems to combine #2115427: Convert language_content_settings_form to FormInterface with something else, which is not clear. That issue has a passing patch, could we just finish that and then rescope this appropriately?
Comment #72
tstoecklerThis change means that $form['#theme'] = 'system_config_form' will no longer be applied. I think it makes more sense to remove the declaration of the save button above and retain the call to parent::buildForm(). The only difference is the label of the button ('Save' vs. 'Save configuration'). Above (around #47 it was decided that we want to leave the button label for now, in order to not break tests (at least I think that was the reason). In that case we should call parent::buildForm() and then override the button label.
Comment #73
tstoecklerAlso #2138151: ContentLanguageSettingsForm is unused (and broken) was marked as a duplicate of this, for reference.
Comment #74
mgiffordJust want to add a note to get back to this #933004: Test that all form elements have a title for accessibility which is postponed based on this issue.
Comment #75
tstoecklerHere's a re-roll for #72. Also noted that #61 was missing #button_type =>primary. So this was not only a Themer's experience issue, but also a UX issue.
Comment #77
tstoeckler75: 1987726-75.patch queued for re-testing.
Comment #79
tstoecklerBeen seeing this a lot lately.
ImageFieldDisplayTest.php, line 229
ImageFieldDisplayTest.php, line 251
array_flip(): Can only flip STRING and INTEGER values!
in FieldableDatabaseStorageController.php, line 212
Comment #80
tstoeckler75: 1987726-75.patch queued for re-testing.
Comment #82
tstoeckler75: 1987726-75.patch queued for re-testing.
Comment #83
tstoecklerWhy do you hate me so much, testbot? I've always tried to be nice to you... :'-(
Comment #85
vijaycs85RE-rolling after #2076445: Make sure language codes for original field values always match entity language regardless of field translatability
Comment #87
tstoecklerThe patches failed on different tests, so just re-uploading the latest patch.
Comment #88
dawehnerIs there a reason why the patch size dropped by 50% from #75 to #85?
Comment #89
tim.plunkettThe changes to content_translation_translatable_batch are gone. But that seems fine, since they weren't really in scope.
Comment #90
Gábor Hojtsycontent_translation_translatable_batch was removed in #2076445: Make sure language codes for original field values always match entity language regardless of field translatability anyway, so no need to cover it anymore :)
Comment #91
Gábor Hojtsy@dawehner, @tim.plunkett: otherwise looks good? :)
Comment #92
kim.pepper87: 1987726-language-controller-85.patch queued for re-testing.
Comment #94
dawehnerShould we not have some kind of service to provide this information for other people?
Comment #95
Gábor HojtsycontentTranslationManager::getSupportedEntityTypes() is the service method to get that from :) The logic there is !empty($info['translatable']) && !empty($info['links']['drupal:content-translation-overview']) so slightly more strict but basically the same.
Comment #96
tstoecklerI don't buy it, Mr. Bot. Re-re-uploading.
Comment #97
tstoecklerComment #98
andypostThis looks like needs API change tag
Comment #99
kim.pepperTagging
Comment #100
vijaycs85Lets test the last patch...
Comment #101
prateek479 CreditAttribution: prateek479 commentedPatch has been rerolled successfully..
Comment #104
Gábor HojtsyComment #105
Gábor Hojtsy101: 1987726-language-controller-100_1.patch queued for re-testing.
Comment #107
XanoRe-roll.
Comment #109
victor-shelepen CreditAttribution: victor-shelepen commentedComment #110
victor-shelepen CreditAttribution: victor-shelepen commentedComment #112
victor-shelepen CreditAttribution: victor-shelepen commented109: language-content_settings_form-1987726-109.patch queued for re-testing.
Comment #113
vijaycs85Ok, it is green and needs review.
Comment #114
sunComment #115
YesCT CreditAttribution: YesCT commentedI'm reviewing this. But it does not apply. So I'm also re-rolling it now. https://drupal.org/patch/reroll
Comment #116
YesCT CreditAttribution: YesCT commentedpatch was removing language_content_settings_page()
and in head there was a change to that function.
patch was expecting to remove line:
return drupal_get_form('language_content_settings_form', language_entity_supported());
in head is now:
return \Drupal::formBuilder()->getForm('language_content_settings_form', language_entity_supported());
because of
#2121175: Remove usage of drupal_get_form()
--
just removing the hunk as in the patch.
(no interdiff since it was a reroll, but that was the difference)
--
attaching patch to see what testbot says and I'll continue to review.
Comment #117
YesCT CreditAttribution: YesCT commentedok, I was wondering if we should keep entitySupported as something like getTranslatableEntities ...
[edit: this was brought up in #94 and #95]
I looked at its one use (which was in the form) and I see we already did the getDefinitions at the beginning, and that entitySupported was doing that again. so, deleting this is ok.
*if* we kept a getTranslatableEntities (aka entitySupported) I guess we would have wanted to do
Nothing needs work so far. Just taking notes.
Comment #118
YesCT CreditAttribution: YesCT commentedread all the comments and summarize the inter-relations of issues relevant here.
Comment #119
YesCT CreditAttribution: YesCT commentedI think we might be able to just get the bundle info for the entities we are interested in the foreach bundle loop below. But that should not be part of this conversion.
Maybe would make a good separate novice issue.
#2241661: Get bundle info for just the entities we need instead of calling getAllBundleInfo() in ContentLanguageSettingsForm
Oh, nevermind. wont fixed that since getBundleInfo calls getAllBundleInfo anyway.
in #75 @tstoeckler added this @todo. Since this @todo is mean to be done after this issue, and not before this issue is rtbc, we need a follow-up issue for it, and to link to it.
Doing that.
And it is listed in the issue summary.
Comment #120
YesCT CreditAttribution: YesCT commentedthis looked a little strange, but I see in #1946404: Convert forms in field_ui.admin.inc to the new form interface it did something similar. In FieldInstanceEditForm in submitForm() it calls ->save and then drupal_set_message().
Comment #121
YesCT CreditAttribution: YesCT commented1.
this was already moved (copied) to buildForm() in #1978916: Convert locale_translate_page to a Controller which had the patch in #111 committed in comment #121
(nothing todo here, just trying to figure out why that code didn't move within this issue)
2.
I was wondering if this would work without this change. I searched the old patches and see this change was introduced by @disasm in #39.
since I am curious, this patch is just to find out.
Comment #122
YesCT CreditAttribution: YesCT commentedthis change is completely unrelated.
it's a whitespace fix.
if we want to do that as a novice follow-up I'm ok with that.
but I'm taking it out of here.
doh! I undid this and did a git diff --color-words just to make sure, and look:
there was a change.
done in #39 by @disasm
to actually fix tests. so leaving this.
Comment #123
YesCT CreditAttribution: YesCT commentedI was doing manual testing but need to go do something else for a few hours.
Comment #125
YesCT CreditAttribution: YesCT commentednote this is the *same* patch as #119. (121 was an experiment to see if it would fail and it did. so 119 is what we want).
manual testing shows the form still works (there are some unrelated things broken about content translation, but nothing I saw related to this issue).
only code change I did while doing this review was adding the issue link for the @todo, so this is rtbc as far as i can tell.
Comment #126
alexpottSo we've just had the form converted but not being used? Funny.
This looks wrong since $entity_type is the object - this change looks unnecessary. In fact there is code that uses $labels that assumes this is a string so this would actually break stuff if we ever add a entity type with out a label.
Comment #127
vijaycs85#126.1 - Yes, it looks like and we are removing this temp callback as part of this issue.
#126.2 - Not sure, how & when but this line has changed somehow from existing code:
Updated now.
Comment #128
andypostLooks good to go, but seems there's no tests for that form.
This change points that there's no tests for that form.
Comment #129
tim.plunkettI think there are tests, just that we don't have any entity types without a label being used here.
Comment #130
Gábor Hojtsy127: 1987726-language-controller-127.patch queued for re-testing.
Comment #131
Gábor HojtsyComment #133
Gábor HojtsyRerolled.
Comment #134
vijaycs85Looks good to me.
The missing test concern(only concern on this issue) raised by @andypost at #128 has been addressed by @tim.plunkett at #129. If we need to create and test an entity without label, it can be done as followup as it might not be the scope of this controller conversion issue.
Comment #135
sunComment #136
alexpottCommitted e0212b0 and pushed to 8.x. Thanks!
Comment #138
Gábor HojtsyWoot, so great to see this land finally. Thanks all for your continued effort!