Updated: Comment #1
Problem/Motivation
Noticed while working on #2045043: Field listings operations cannot be altered, it puts the alter link there,
and config_translation has the right translate link working,
for example
admin/structure/types/manage/article/fields/node.article.body/field
but the link added in the operations alter is
entity/field_instance/node.article.body/translate
Proposed resolution
Implement uri in field instance,
similar to how it is done in blocks.
Remaining tasks
TBD
User interface changes
NA
API changes
No.
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#29 | 2057227-field_instance_needs_uri-29.patch | 977 bytes | vijaycs85 |
#29 | 2057227-diff-26-29.txt | 760 bytes | vijaycs85 |
#26 | 2057227-field_instance_needs_uri-26.patch | 1017 bytes | vijaycs85 |
#26 | 2057227-diff-24-26.txt | 884 bytes | vijaycs85 |
#24 | 2057227-field_instance_needs_uri-24.patch | 915 bytes | vijaycs85 |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedat drupalcamping @webflo had the idea for the fix and helped me do it
Manually tested with config_translation module on body field on article and the picture field on the user (admin/config/people/accounts/fields)
used as a basis:
with the way it was doing similar thing in config_translation, using the entity manager and getting the admin path:
I did not
use Drupal;
because elsewhere in FieldInstance it has \Drupal::entityManager() in saveNew.
should we inject it and change it everywhere in this class?
seems more than this issue is about.
Comment #2
webflo CreditAttribution: webflo commentedMaybe we should check if field_ui is enabled. Because field_ui module provides the page controller for "admin/structure/types/manage/article/fields/node.article.body/field". On the other hand the generic url (entity/field_instance/node.article.body) from Entity::uri is also not correct.
Comment #3
webflo CreditAttribution: webflo commentedWe have same issue with views and views_ui. I think the patch is okay.
Comment #4
Gábor HojtsyLooks much like #2044825: Language entity missing uri() method implementation. I see the entity manager is not injected, but neither is anywhere else in this class as pointed out. This is a blocker to integrate fields with config translation (#2045077: Field translatability not wired up on the UI).
Comment #5
vijaycs85Injecting entityManager and created new issue to replace other \Drupal:: in this class at #2058983: Provider getter for dependencies in FieldInstance class
Comment #6
vijaycs85Minor replacement issue in #5
Comment #7
Gábor HojtsyI don't see how this is an improvement. It just takes the same entity manager service globally at a different point. It is still not injectable. This does not allow for anything more than #1 as far as I see. Am I missing something?
Comment #8
Gábor HojtsyRetitle to be better :)
Comment #9
vijaycs85As discussed with @ dawehner, @katbailey and @timplunkett on #drupal-wscci, updating code to provide setting to make it more unit-testable. We can't inject like we do in controller as entity shouldn't depend on service data(For more details, refer IRC conversation.
Comment #10
tim.plunkettNot needed
How about storing it in a protected property for later?
Comment #11
vijaycs85Something like https://gist.github.com/vijaycs85/6213717 ?
Comment #12
vijaycs85Thanks for the quick response @tim.plunkett on IRC. here is the updated patch for #10
Comment #13
tim.plunkettPatch looks good. Does this need test coverage?
Comment #14
dawehnerI am wondering whether there should be also a setter, so you can actually set the entity manager, for example for unit tests?
Comment #16
BerdirHm. FieldInstance is an entity, creating some kind of custom setter/getter stuff here is IMHO wrong.
If you want to inject stuff, fix #2015535: Improve instantiation of entity classes and entity controllers :)
Comment #17
Gábor Hojtsy#2045077: Field translatability not wired up on the UI has tests that would start passing once this lands. It will test the alterability of the operations in field listings and use the uri() method. So it will be somewhat of an integration test for this at least.
Looks like either way we don't want / cannot do the classic injection approach and there is already an issue to do something about that that has existing activity. So we can close this down focusing on the original problem only without going down on a rathole.
Looks like the patch fails on dblogtest, so looks unlikely. Will send to retest.
Comment #18
Gábor Hojtsy#12: 2057227-field_ui-url-12.patch queued for re-testing.
Comment #19
Gábor HojtsyYay, the fail was indeed a fluke. RTBC as per above:
- test coverage will be/is included with config_translation (#2045077: Field translatability not wired up on the UI will start passing as soon as this lands :)
- making entity controller injectable is happening in #2015535: Improve instantiation of entity classes and entity controllers which already has its history and activity
- purpose of this issue is fulfilled with the patch and worked out with tim.plunkett, katbailey, etc.
Comment #20
BerdirNot sure if we shouldn't just go ahead with #1. I think that introducing a one-off solution for a single entity type will either mean that we have to remove this again when the other issue happens or we'll have an inconsistent behavior between different entity types.
Comment #21
Gábor HojtsyAll right, #1 is not functionally different at all. All further work was about exploring and then failing to find good ways to inject the entity manager, which is in fact already covered in #2015535: Improve instantiation of entity classes and entity controllers and will handle this if this lands earlier. Reuploading #1 then.
Comment #22
andypost$path could be empty and field_ui module could be disabled so this needs more testings
Comment #23
Gábor Hojtsy@andypost: in case getAdminPath() gets us an empty string, do you suggest we fall back on the base implementation where we extend from? That will result in 'entity/' . $this->entityType . '/' . $this->id() which is not true / useful either. It is a random useless path either way. See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...
We can do that at least and then not return a path that starts with a '/fields' I guess. So we'd return a concrete path but it would be equally useless. It would not change the behaviour at least compared to prior the patch in that case :)
As for when field UI is disabled, the entity classes are not equipped (and I think don't want to care about) module functionality pairing. I mean none of the other modules test if the module is turned on that the path would end up being served by. Eg. a View does not check if Views UI is enabled, it just returns the URI assuming it is: https://api.drupal.org/api/drupal/core%21modules%21views%21lib%21Drupal%... - otherwise it could only return a useless URI right? The URI method on the entity interface does not allow for returning special things on "error": https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21... so it assumes you can produce a URI at all times. As evidenced by the base implementation it cares for providing an URI more than it being useful for anything or being actually served by anything.
So marking for needs work based on the need to fall back on parent::uri() if getAdminPath() returns an empty string.
Comment #24
vijaycs85not sure I got it right, but does this patch help?
Comment #25
Gábor HojtsyI don't think that is right, the parent URI is the URI of the entity, while what you do is you add even more stuff onto it later here. If we cannot compute the field UI path of the entity, we should fall back on the default (crappy) URL in full (and nothing else) from the parent.
Comment #26
vijaycs85Thanks for the quick review @Gábor Hojtsy. here is the fix.
Comment #27
Gábor HojtsyLooks all good to me! Anybody else agree? :)
Comment #28
tim.plunkettOne liner, please
Comment #29
vijaycs85Sure @tim.plunkett
Comment #30
tim.plunkettLooks good to me.
Comment #32
Berdir#29: 2057227-field_instance_needs_uri-29.patch queued for re-testing.
Comment #33
BerdirTestbots having a bad day? Missing table random fail.
Comment #34
webchickJust a question. Should this be part of the interface for FieldInstanceInterface so it's required for classes to implement this method?
Also, if this is fixing a problem, should there not be tests for this? Or is that being covered in #1952394: Add configuration translation user interface module in core?
Comment #35
tim.plunkettIt's on the Entity interface, it just provides a default implementation.
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...
That's what we're overriding.
Comment #36
Gábor HojtsyYeah, also #1952394: Add configuration translation user interface module in core adds tests for alterability of all the listings, to ensure it can expose the translation UI and its actually accessible (because of all those needed a long march), it does cover this too. We don't have that in that patch because it would fail due to this issue :D We have the patch prepped fully in #2045077: Field translatability not wired up on the UI which only awaits for this to get committed and it will stop failing :)
Comment #37
webchickOk great, thanks!
Committed and pushed to 8.x.
Comment #38
Gábor HojtsyYay, thanks!
Comment #39.0
(not verified) CreditAttribution: commentedtaking out related, it's mentioned in the first sentence. and it is in the list.