This is at least dependent on #2045043: Field listings operations cannot be altered but then also the field instance does not have a URL properly. If the field instance knows which entity it belongs to, maybe we can have a url() method on it that would return an entity specific URL which would make this wired up?

Also needs tests. This is a clear blocker for core inclusion IMHO because it will uncover API changes needed once we have the field translatability in place.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

The field instance url() method would be similar to #2044825: Language entity missing uri() method implementation but it needs to take the parent entity into account.

Gábor Hojtsy’s picture

Also it would need to be one more core issue to add that too.

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
2.35 KB

Let's start with a test. Now we can at least see the translate operations (but the links are not right). We still need the URL method on field instances solved.

Status: Needs review » Needs work

The last submitted patch, field-list-tests.patch, failed testing.

Gábor Hojtsy’s picture

The fields URI problem already has an issue in #2057227: Field instance needs uri() method different from the default. That should be fixed for this to fully be possible, but we need the test here fixed as well :) The custom block module is not enabled.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
2.62 KB

Fixed field issue, if the path issue resolved, it should work now.

Status: Needs review » Needs work

The last submitted patch, 2045077-field-list-6.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Postponed

patch in #6 failing because of #2057227: Field instance needs uri() method different from the default after applying the patch there, tests passing locally. So postponing this issue until we get #2057227: Field instance needs uri() method different from the default in.

Gábor Hojtsy’s picture

Status: Postponed » Needs review
Issue tags: -Needs tests, -D8MI, -language-config

#6: 2045077-field-list-6.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2045077-field-list-6.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +D8MI, +language-config

#6: 2045077-field-list-6.patch queued for re-testing.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

The last submitted patch, 2045077-field-list-6.patch, failed testing.

Gábor Hojtsy’s picture

Ok, now we only have our own fails! Article node type undefined. Makes sense for a test :) We should create it. @vijaycs85 are you on this?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.61 KB

Here is an updated version. Article is not a content type created by default but if we create a content type, the body field is added. So adding the body field on article was the wrong solution. We should add a custom content type, like other tests do and it will get a body field.

Status: Needs review » Needs work

The last submitted patch, 2045077-field-list-15.patch, failed testing.

Gábor Hojtsy’s picture

FileSize
2.61 KB

Wups, some fixes were missing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2045077-field-list-17.patch, failed testing.

Gábor Hojtsy’s picture

FileSize
3.29 KB

Tried to debug why the page router does not get registered. I *thought* it is due to hook_menu, that will not get regenerated when a new content type is added (since we include entity based field URLs static in the list for hook_menu). However converting this to hook_menu_alter() did not seem to solve it :/ Any better ideas? This is the only fail now :) @vijaycs85?

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2045077-field-list-19.patch, failed testing.

YesCT’s picture

Assigned: Unassigned » YesCT
Status: Needs work » Needs review
FileSize
2.55 KB
2.75 KB

I was reading this through. Just some style stuff while I think about it. Also, reverted the change to the alter hook, since it didn't seem to make a difference. Is there another reason to make it be an alter?

in #17 it said some fixes missing... but the patch is the same. Was a file not added to the patch?

Status: Needs review » Needs work

The last submitted patch, 2045077-field-list-23.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

tried this manually, and the translate link works for body field on article.

installed in minimal.
------
and found this unrelated error. noting it here to look into later:
on:

/admin/structure

Notice: Undefined offset: 4 in _menu_translate() (line 766 of core/includes/menu.inc).
Symfony\Component\Routing\Exception\ResourceNotFoundException: The route for '/admin/structure/contact/manage//translate' could not be found in Drupal\Core\Routing\RouteProvider->getRouteCollectionForRequest() (line 103 of /Users/ctheys/foo/d8/core/lib/Drupal/Core/Routing/RouteProvider.php).

-----

when installed a minimal profile, and manual create a content type, I dont get body created automatically.
why does it appear to be created in the test?

maybe I need to enable the fields module
yep. had to enable field ui.

now I have a manages fields operation and a body.
...and it is fine manually going to the translate link on the body.

Hm..

Will look at this more later.

vijaycs85’s picture

#6: 2045077-field-list-6.patch queued for re-testing.

Gábor Hojtsy’s picture

@YesCT: I've opened #2068819: Problem with contact category translation routes for your contact bug. Just so its preserved.

Gábor Hojtsy’s picture

#6: 2045077-field-list-6.patch queued for re-testing.

vijaycs85’s picture

Thanks @Gábor Hojtsy for great help on IRC. Here is the updated field URL version which fixes the test issues.

Gábor Hojtsy’s picture

Status: Needs review » Fixed

Yay! Thanks for working this out with me :) Committed and pushed. Also removed the @todo as well since the real test passes now. Yay!

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