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.

Files: 
CommentFileSizeAuthor
#29 2057227-field_instance_needs_uri-29.patch977 bytesvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,191 pass(es). View
#29 2057227-diff-26-29.txt760 bytesvijaycs85
#26 2057227-field_instance_needs_uri-26.patch1017 bytesvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,082 pass(es). View
#26 2057227-diff-24-26.txt884 bytesvijaycs85
#24 2057227-field_instance_needs_uri-24.patch915 bytesvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 57,782 pass(es). View
#24 2057227-diff-21-24.txt764 bytesvijaycs85
#21 drupal8.field-ui.module.uri_.2057227-1.patch898 bytesGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 57,790 pass(es). View
#12 2057227-field_ui-url-12.patch2.53 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,030 pass(es). View
#12 2057227-diff-9-12.txt1.38 KBvijaycs85
#9 2057227-field_ui-url-9.patch2.5 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#9 2057227-diff-6-9.txt2.74 KBvijaycs85
#6 2057227-field_ui-url-6.patch2.56 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 57,623 pass(es). View
#6 2057227-diff-5-6.txt2.48 KBvijaycs85
#5 2057227-field_ui-url-5.patch2.56 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 57,150 pass(es), 72 fail(s), and 12 exception(s). View
#5 2057227-diff-1-5.txt2.48 KBvijaycs85
#1 drupal8.field-ui.module.uri_.2057227-1.patch898 bytesYesCT
PASSED: [[SimpleTest]]: [MySQL] 57,554 pass(es). View

Comments

YesCT’s picture

Status: Active » Needs review
FileSize
898 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,554 pass(es). View

at 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:

  /**
   * Overrides \Drupal\Core\Entity\Entity::uri();
   */
  public function uri() {
    return array(
      'path' => 'admin/structure/block/manage/' . $this->id(),
      'options' => array(
        'entity_type' => $this->entityType,
        'entity' => $this,
      ),
    );
  }

with the way it was doing similar thing in config_translation, using the entity manager and getting the admin path:

  $entity_manager = Drupal::entityManager();
  foreach ($entity_manager->getDefinitions() as $entity_type => $entity_info) {
    // Make sure entity type is fieldable and has base path.
    if ($entity_info['fieldable'] && isset($entity_info['route_base_path'])) {
      // Get all available bundles available for this entity type.
      foreach (entity_get_bundles($entity_type) as $bundle => $bundle_info) {
        if ($path = $entity_manager->getAdminPath($entity_type, $bundle)) {
          debug($path);
          $items[] = new ConfigEntityMapper($path . '/fields/{field_instance}' , 'field_instance', t('@label field'), MENU_CALLBACK);

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.

webflo’s picture

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.

webflo’s picture

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +sprint, +blocker

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

vijaycs85’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.48 KB
2.56 KB
FAILED: [[SimpleTest]]: [MySQL] 57,150 pass(es), 72 fail(s), and 12 exception(s). View

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

vijaycs85’s picture

FileSize
2.48 KB
2.56 KB
PASSED: [[SimpleTest]]: [MySQL] 57,623 pass(es). View

Minor replacement issue in #5

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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?

Gábor Hojtsy’s picture

Title: Field Instance url needs own method instead of Entity » Field instance needs uri() method different from the default

Retitle to be better :)

vijaycs85’s picture

FileSize
2.74 KB
2.5 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

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.

tim.plunkett’s picture

Status: Needs work » Needs review
@@ -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?

vijaycs85’s picture

How about storing it in a protected property for later?

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

vijaycs85’s picture

FileSize
1.38 KB
2.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,030 pass(es). View

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

tim.plunkett’s picture

Patch looks good. Does this need test coverage?

dawehner’s picture

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

Status: Needs review » Needs work

The last submitted patch, 2057227-field_ui-url-12.patch, failed testing.

Berdir’s picture

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

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review

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

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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.

Berdir’s picture

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.

Gábor Hojtsy’s picture

FileSize
898 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,790 pass(es). View

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.

andypost’s picture

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
764 bytes
915 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,782 pass(es). View

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

Gábor Hojtsy’s picture

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.

vijaycs85’s picture

FileSize
884 bytes
1017 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,082 pass(es). View

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

Gábor Hojtsy’s picture

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

tim.plunkett’s picture

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

One liner, please

vijaycs85’s picture

FileSize
760 bytes
977 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,191 pass(es). View

Sure @tim.plunkett

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

Status: Reviewed & tested by the community » Needs work
Issue tags: -D8MI, -sprint, -language-config, -blocker

The last submitted patch, 2057227-field_instance_needs_uri-29.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +sprint, +language-config, +blocker
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Testbots having a bad day? Missing table random fail.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

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?

tim.plunkett’s picture

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.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok great, thanks!

Committed and pushed to 8.x.

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

taking out related, it's mentioned in the first sentence. and it is in the list.