Problem
The configuration translation user interface module needs field operations to be alterable, so we can add a translation operation on field configuration screens. Altering operations is possible for other entity listings, fields are not implemented as entity list controllers.
Proposal
Make operations altering possible. We don't need to convert this to a full on entity list controller to do it, we just need to let the operations be altered.
Comment | File | Size | Author |
---|---|---|---|
#31 | interdiff-26-31.txt | 2.8 KB | bechtold |
#31 | 2045043-filter-alter-31.patch | 2.52 KB | bechtold |
#26 | 2045043-filter-alter-26.patch | 2.04 KB | bechtold |
#23 | interdiff-20-23.txt | 568 bytes | bechtold |
#23 | 2045043-filter_alter-23.patch | 568 bytes | bechtold |
Comments
Comment #1
Gábor HojtsyInitial patch. This would need the module handler injected I think, definitely not just call out to \Drupal simply like it does now :D However this one works. Can someone help with doing the injection of the module handler?
Comment #2
Gábor HojtsyComment #3
vijaycs85trying to inject module handler...
Comment #5
Gábor HojtsyWhat do we need the entity manager for?
The error reported by testbot is: Argument 1 passed to Drupal\field_ui\FieldOverview::__construct() must be an instance of Drupal\field_ui\EntityManager, instance of Drupal\Core\Entity\EntityManager given. There should be a "use" for Drupal\Core\Entity\EntityManager.
Comment #6
vijaycs85Seems EntityManager is used in submit.
Comment #7
vijaycs85Updating tag.
Comment #8
amateescu CreditAttribution: amateescu commentedThe thing is.. we do want to turn that screen into an EntityListController, and that should happen in #1963340: Change field UI so that adding a field is a separate task.
Comment #9
amateescu CreditAttribution: amateescu commentedSorry for eating the tag.
Comment #10
vijaycs85Updating sprint tag - LONDON_2013_JULY
Comment #11
Gábor HojtsyFrom the point of view of field config translation, we want to wire it up to this UI but we cannot do it ATM. Unless lack of field config translatability is at most a normal bug, we cannot get the module into core without this present.
Comment #12
yched CreditAttribution: yched commentedGetting this in separately, and then adjust the code if/when the "manage fields" is turned to a ListController, sounds fine by me ?
Why are we also adding an entity manager ? Doesn't look like it's used anywhere (nor documented) ?
Comment #13
YesCT CreditAttribution: YesCT commentedI looked this over. I thought we should have it ready, in case we decide to go ahead with it.
Only thing I could find was missing descriptions for @params
and I put the new use statements as close to alphabetical as I could, only changing the lines already changed.
Everything else looks fine. I think I might be starting to understand all the things to check with injection in terms of the __contstruct, create, having the parameters match and the order of things set in the static.
the entitymanager property comes from OverviewBase
--
I didn't try this with config_translation yet.
Comment #14
YesCT CreditAttribution: YesCT commented@yched
I asked myself the same question about the entitymanager.
we are not using it in the new code in this patch, but it was already used elsewhere.
For example, in the submitForm()
And that was working before this patch added it, because it was inheriting the property and the __construct and create from OverviewBase since
class FieldOverview extends OverviewBase
(At least that is what I think... let me know if otherwise.)
Comment #15
YesCT CreditAttribution: YesCT commentedtested it with config_translation without and with the patch. :)
Comment #16
yched CreditAttribution: yched commentedAh, right, entityManager is defined and documented in the parent class.
No biggie, but I think then it would be slightly more correct if FieldOverview::__construct() did:
Other than that, looks good to me :-)
Comment #17
YesCT CreditAttribution: YesCT commented@yched That makes sense, then if the construct there changes, we get those changes here automatically.
My syntax was a bit different, but did #16.
Hopefully this will be ready now.
Comment #18
yched CreditAttribution: yched commentedThis looks good if it comes back green.
Comment #19
YesCT CreditAttribution: YesCT commentedgreen. cool.
Comment #20
bechtold CreditAttribution: bechtold commentedrerolled because @YesCT suggested to do it :-)
There was a Conflict in core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php.
So i added
use Drupal\Core\Entity\Field\FieldTypePluginManager;
Comment #22
bechtold CreditAttribution: bechtold commentedanother try, couldn't find a php synax error, but fixed the formatting a bit.
Comment #23
bechtold CreditAttribution: bechtold commentedin #22 too much formatting was done.
Comment #24
bechtold CreditAttribution: bechtold commentedin #22 too much formatting was done.
Comment #26
bechtold CreditAttribution: bechtold commenteduploaded wrong patch in #23
Comment #27
bechtold CreditAttribution: bechtold commentedneeds review
Comment #29
yched CreditAttribution: yched commentedThe patch tries to add a second __construct() method while there is already one :-)
Comment #30
YesCT CreditAttribution: YesCT commented@bechtold is here at the camp, so I will see if they can do it tonight.
Comment #31
bechtold CreditAttribution: bechtold commented#2047993-23: Remove current uses of field_info_*_types() / field_info_*_settings() added the same constructor and create method as our rerolled patch #26.
We fixed it.
Comment #33
YesCT CreditAttribution: YesCT commentedFAILED: [[SimpleTest]]: [MySQL] 57,920 pass(es), 2 fail(s), and 0 exception(s).
HTTP response expected 204, actual 400 Browser NodeTest.php 70 Drupal\rest\Tests\NodeTest->testNodes()
Value '0\'v;Vlbc' is equal to value '|~-iJ\\1`'. Other NodeTest.php 74 Drupal\rest\Tests\NodeTest->testNodes()
I think that is a random failure. (#2056713: Random test failure in Drupal\rest\Tests\NodeTest)
Comment #34
YesCT CreditAttribution: YesCT commented#31: 2045043-filter-alter-31.patch queued for re-testing.
Comment #36
YesCT CreditAttribution: YesCT commented#31: 2045043-filter-alter-31.patch queued for re-testing.
Comment #37
yched CreditAttribution: yched commentedRight, I forgot that #2047993: Remove current uses of field_info_*_types() / field_info_*_settings() would conflict. Thanks for the detective work!
Comment #38
YesCT CreditAttribution: YesCT commentedwhile working on this testing I noticed a different problem.
#2057227: Field instance needs uri() method different from the default
Comment #39
alexpottThe new hook needs an example in field.api.php
Also we really ought to have a test implementation - ie. a test module in the field/tests/modules that provides an implementation that can be tested by a test in the field module
Comment #40
alexpottComment #41
amateescu CreditAttribution: amateescu commentedNot really..
So, the patch is a bit weird because it invokes an already existing hook (!) that is usually invoked by the EntityListController, but since the field list is not yet a list controller, Gabor wants to have this "shortcut" for the config translation module until #1963340: Change field UI so that adding a field is a separate task happens.
The hook added here is actually https://api.drupal.org/api/drupal/core!includes!entity.api.php/function/..., and it's already tested by https://api.drupal.org/api/drupal/core!modules!system!tests!modules!enti...
Comment #42
Gábor HojtsyTag as blocker for config translation as per above.
Comment #43
alexpottThanks for answering my questions @amateescu
Committed faaaaae and pushed to 8.x. Thanks!
Comment #44
yched CreditAttribution: yched commentedNice git hash.
Comment #45
Gábor HojtsyRemove sprint tag.