Field instance needs uri() method different from the default

Maybe 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.

We have same issue with views and views_ui. I think the patch is okay.

Looks 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).

Injecting entityManager and created new issue to replace other \Drupal:: in this class at #2058983: Provider getter for dependencies in FieldInstance class

Minor replacement issue in #5

I 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?

Retitle to be better :)

As 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.

@@ -272,6 +272,7 @@ public function __construct(array $values, $entity_type = 'field_instance') {
     }
 
+
     // 'Label' defaults to the field ID (mostly useful for field instances

Not needed

@@ -624,4 +639,13 @@ public function unserialize($serialized) {
+    return \Drupal::entityManager();

How about storing it in a protected property for later?

How about storing it in a protected property for later?

Something like https://gist.github.com/vijaycs85/6213717 ?

Thanks for the quick response @tim.plunkett on IRC. here is the updated patch for #10

Patch looks good. Does this need test coverage?

I am wondering whether there should be also a setter, so you can actually set the entity manager, for example for unit tests?

Hm. 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 :)

#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.

#12: 2057227-field_ui-url-12.patch queued for re-testing.

Yay, 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.

Not 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.

All 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.

@@ -540,6 +540,21 @@ public function isFieldTranslatable() {
+    $path = $entity_manager->getAdminPath($this->entity_type, $this->bundle);
...
+      'path' => $path . '/fields/' . $this->id(),

$path could be empty and field_ui module could be disabled so this needs more testings

@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.

not sure I got it right, but does this patch help?

I 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.

Thanks for the quick review @Gábor Hojtsy. here is the fix.

Looks all good to me! Anybody else agree? :)

@@ -537,6 +537,27 @@ public function isFieldTranslatable() {
+    $entity_manager = \Drupal::entityManager();
+    $path = $entity_manager->getAdminPath($this->entity_type, $this->bundle);

One liner, please

Sure @tim.plunkett

Looks good to me.

#29: 2057227-field_instance_needs_uri-29.patch queued for re-testing.

Testbots having a bad day? Missing table random fail.

Just 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?

It'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.

Yeah, 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 :)

Ok great, thanks!

Committed and pushed to 8.x.

Yay, thanks!