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.
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.
Comment | File | Size | Author |
---|---|---|---|
#66 | language_admin_overview-2005778-66.patch | 12.06 KB | maggo |
#66 | interdiff.58-66.txt | 735 bytes | maggo |
#60 | language_admin_overview-2005778-60.patch | 11.92 KB | naxoc |
#60 | interdiff.txt | 949 bytes | naxoc |
#58 | interdiff.54-58.txt | 1.89 KB | penyaskito |
Comments
Comment #1
plopescFirst approach.
Comment #2
naxoc CreditAttribution: naxoc commentedI'll take a look at this today at the Dev Days sprint.
Comment #3
naxoc CreditAttribution: naxoc commentedHere is a reroll of #1
Comment #4
Gábor HojtsyPatch looks pretty good. The code moved around seems identical.
Should this also implement a controller interface (or extend from a base form controller) since it is a form controller after all? :) I mean not just implement this interface?
Comment #5
naxoc CreditAttribution: naxoc commentedUpdated patch. The form controller now inherits from a controller interface.
Would it make better sense to make a base form controller for it?
Comment #6
Gábor HojtsyLooked through other FormInterface implementations. This seems like well enough based on those. There is also no need to inject something here given no outside services used. So looks good!
Comment #7
naxoc CreditAttribution: naxoc commentedJust talked to Gabor and we found that the implementation of a controller interface is not necessary after all.
This is just the patch from #3
Comment #8
Gábor Hojtsy@katbailey also explains we only need the controller interface implemented if we are to inject something and as per #6 we don't have. So #3 should be fine.
Comment #9
Gábor HojtsyJup, #7 just a repost of #3, still RTBC :)
Comment #10
alexpottWe shouldn't be using check_plain here...
String::checkPlain()
Comment #11
webflo CreditAttribution: webflo commentedConverted check_plain to String::checkPlain().
Comment #12
Gábor HojtsyLooks good!
Comment #13
webflo CreditAttribution: webflo commentedTestbot got stuck. We test it again.
Comment #14
Gábor HojtsyComment #15
tim.plunkettWhen #1754246: Languages should be configuration entities goes in, this should actually be a EntityListController. See RoleListController as an example.
Comment #16
Gábor HojtsyWe are aware of that, discussed with @alexpott too.
Comment #17
alexpottThis now needs to be an entity list controller as #1754246: Languages should be configuration entities has landed
Comment #18
plopescUnassign
Comment #19
webflo CreditAttribution: webflo commentedRerolled.
Comment #20
Gábor HojtsyAs said above, now that #1754246: Languages should be configuration entities is in, this patch should extend from entity list controller and don't produce the data for the list itself. Much like any other entity list controller in contact, roles, etc.
Comment #21
andypostRelated #2031277: Implement checkCreateAccess on LanguageAccessController
Comment #22
Gábor HojtsyThis unfortunately also does not use the buildHeader(), getOperations(), buildOperations(), etc. patter that is required to support #2004428: Less ugly operations altering in config_translation. It should use the proper method separations that are in config entity list controllers generally.
Comment #23
naxoc CreditAttribution: naxoc commentedComment #24
tim.plunkettActually, this isn't listing Drupal\language\Plugin\Core\Entity\Language, which would use the EntityListController.
It's actually listing Drupal\Core\Language\Language objects.
Comment #25
naxoc CreditAttribution: naxoc commentedHere is a patch that uses EntityListController.
It looks to me like it lists Drupal\language\Plugin\Core\Entity\Language - I had no trouble with listing them.
Comment #27
tim.plunkettThis is a list of Drupal\Core\Language\Language objects
This is Drupal\language\Plugin\Core\Entity\Language.
When I looked at this earlier, it would show zxx and und languages as well.
Comment #28
Gábor HojtsyTim is right that locked languages should not be shown. Rows where locked = 1 should be skipped as per prior to this patch. Also, the submission function should not use language_list(), so it is self contained in terms of the entity management it does. language_list() works with a processed list of languages, the form submission should work with entities direct.
Comment #29
tim.plunkettYou can override the EntityListController::load() like this:
Comment #30
Gábor HojtsyWow, fancy!
Comment #31
naxoc CreditAttribution: naxoc commentedI am having trouble saving the weights on the languages without using language_list().
I tried calling save() on the entities, but then the names disappear.
What would be the best way to save the weights?
Comment #32
webflo CreditAttribution: webflo commentedHmm i can not reproduce this problem. But its required to kill the static cache in language_list and call language_update_locked_weights() to updated the non configurable languages.
Comment #33
penyaskitoI'm wondering if we could refactor language_save() and this controller, so we shouldn't need to duplicate the cache-clearing + updating of locked weights. That could be done in a follow-up and I wouldn't consider that a blocker for RTBCing.
If there is no validation, could we remove this?
Comment #34
penyaskitoAnswering myself: validateForm is needed for implementing FormInterface and not provided for any parent classes.
I would mark this as RTBC, but would be great having other two eyes.
Comment #35
andypostOtherwise good
_entity_list = 'language_entity'
Comment #36
webflo CreditAttribution: webflo commentedSo we should add both? EntityListController::listing needs $entity_type as argument.
Comment #37
webflo CreditAttribution: webflo commentedOkay i got it.
Comment #38
andypostThat's should be better
Comment #39
andypostReverted back to languages because of
locale_form_language_admin_overview_form_alter()
Comment #40
webflo CreditAttribution: webflo commentedLangauge entities dont use the common URL structure form Drupal core (entity/{id}/edit). I makes no sense to use uri() and generated edit/delete links at this point. We can convert this urls in a follow-up.
Comment #42
penyaskitoI'm working on a fix for this, patch will follow.
Comment #43
penyaskitoMy first try to get it back to green finished with something very similar to #43, so I discarded and tried something different.
For reusing buildOperations, uri() and access() were required in the entity.
In the case of configuration entities, default edit path is the MENU_DEFAULT_LOCAL_TASK but in this case IMHO does not make sense, so I had to workaround it. For making this work was necessary to modify edit and delete paths in hook_menu, but I think that this is something desired anyway sooner or later for consistency with other entities.
In locale_form_language_admin_overview_form_alter we have to ensure that the weight is the last column, but anyway the toggle of visibility of the weight column is not working because the #links in operations. I couldn't find an issue for that bug, but IMHO this should be fixed instead of doing workarounds.
Comment #45
penyaskitoFinishing url conversion.
Comment #47
penyaskitoOops, missed one.
Comment #48
Gábor HojtsyAll (almost all?) other config entities in core now use the ...../manage/.... pattern in the paths. It is true languages don't do that. If we *need* to change the paths here, we should rather change to that. If we can avoid changing the paths for the controller, that would probably be better just for the sake of making it easier to review.
Comment #50
webflo CreditAttribution: webflo commentedBased on patch from comment 37.
- Fixed the drag and drop problem
- Added LanguageAccessController
Comment #51
andypostTake example form menu access patch #2012916: Implement access controller for the menu and menu link entity
this should be $operation specific
Comment #52
vijaycs85Added LanguageAccessController in #2031277: Implement checkCreateAccess on LanguageAccessController. Same code updated here.
Comment #53
penyaskitoThe $entity is already the $language, why loading it again?
Comment #54
vijaycs85:) same comment from @andypost at #2031277-3: Implement checkCreateAccess on LanguageAccessController. Updating here.
Comment #55
YesCT CreditAttribution: YesCT commentededit is missing from the languages table.
I looked around, but haven't figured out why yet.
Comment #56
YesCT CreditAttribution: YesCT commentedWhile reading this, I'd recommend these changes (if they are accurate).
scheme momentarily confused me with schema. :) so I think we can just say pattern.
And "then" is about time. "than" is a comparison. So we want "than" here.
Some comment about why the site default is has a special check would be good.
Comment #57
naxoc CreditAttribution: naxoc commentedI am wondering why we call the parent
getOperations, buildHeader(),
andbuildRow
just to remove stuff from the returned values promptly? It feels to me like we loose control over what we display if the parent decides to throw in stuff we don't want?Comment #58
penyaskitoEdit should be available now, the permission is "update" and not "edit".
Fixed comments said in #56.
@naxoc: We are only unsetting the id because it's not very user-friendly to show the langcode in this case, and instead we have the language name.
Comment #59
tim.plunkettWe need to call buildOperations() for the hook_entity_operations_alter() to fire.
The rest, no need to call parent.
Comment #60
naxoc CreditAttribution: naxoc commentedHere is a patch that does nothing but remove the unnecessary calls to parent().
Comment #62
penyaskitoIf we remove the calls to parent, we need to include the operations from there. I consider #58 much cleaner than adding this code duplication.
Comment #63
YesCT CreditAttribution: YesCT commentedin access() are these operations, or permissions? It is confusing because in other places, the operation is edit.
====
Seems like we can either use parent, and then remove stuff,
or
not use parent, and add specifically the stuff from parent that we want.
If, something is added to parent that we want to inherit, and we dont use parent, we have to update things here to, so I think would would need an @see in the parent pointing to this, so we know when that changes that we should change things here?
Then I suppose the opposite is true too.
Do we have another place in core for something similar happening that we can use as a guide for what approach to take?
Maybe using the parent pattern is already a common way to deal with this situation.
Also.. #58 was green.
Comment #64
penyaskitoThe operations for the access methods are create, update, delete. I checked CommentAccessController for another example.
About another example, in VocabularyListController you can find that we are using the same strategy than here.
Comment #65
YesCT CreditAttribution: YesCT commentedYep. I checked MenuListController and MenuAccessController too. it's update operation in access and edit operation in list. :)
Comment #66
maggo CreditAttribution: maggo commentedAdded create operation in LanguageAccessController, based on #58
Comment #67
maggo CreditAttribution: maggo commentedComment #68
webflo CreditAttribution: webflo commentedLooks good. The new access controller for language entities is solid and we can continue with other language admin pages after this got committed.
- #2038291: Convert language_admin_add_form to a Controller
- #1946426: Convert all of confirm_form() in language.admin.inc to the new form interface
- #2003592: Convert language_admin_add_form and language_admin_edit_form to a Controllers
Comment #69
YesCT CreditAttribution: YesCT commented#66: language_admin_overview-2005778-66.patch queued for re-testing.
Comment #70
YesCT CreditAttribution: YesCT commentedI can confirm that with this applied, that the config_translation module puts a Translate link in the operations of
admin/config/regional/language
Comment #71
alexpottCommitted 9add7cb and pushed to 8.x. Thanks!
Comment #72
Gábor HojtsyYay!
Comment #74
penyaskito