Toolbar doesn't show the "Edit" tab: fix by using new entity display settings, add tests

I can confirm on a clean install, I dont have the "Edit" menu item.

Please provide steps to reproduce.

Crosspost.

@Bojhan: thanks!

@aspilicious: I'd like to get steps to reproduce from you as well, if you have a different way to reproduce it, that would help significantly in tracing the root cause.

#4: And you created some content? Without any content, there is nothing to edit, and the "Edit" tab will not appear.

1. I did a clean D8 HEAD install.
2. Created an article (while still logged in as uid 1).
3. The "Edit" tab appears (while still logged in as uid 1), in Chrome & Firefox.

Very odd :/

Can you verify in your browser's developer tools that an AJAX request is made to edit/metadata, which should return some JSON similar to this:

{"node:1:field_image:und:full":{"label":"Image","access":true,"editor":"form","aria":"Entity node 1, field Image"},"node:1:body:und:full":{"label":"Body","access":true,"editor":"direct-with-wysiwyg","aria":"Entity node 1, field Body","format":"filtered_html","formatHasTransformations":false},"node:1:field_tags:und:full":{"label":"Tags","access":true,"editor":"form","aria":"Entity node 1, field Tags"}}

I'm on PHP 5.3.8, but I very, very much doubt PHP version differences will cause this.

It's always there, but only if something editable exists on the page, it gets unhidden. The JSON of #9 is what tells Edit's JS whether something editable exists on the page.

I'm not sure how to write a test for this, but here is the code fix.

It seems more reasonable to account for the cases where the field is hidden, even though it's not supposed to happen ?

OK, drop that.

What we need is a formatter plugin_id.
What the current code does is build the data-edit-id value in the original HTML request based on the $view_mode. Then the ajax 'metadata' request has to re-run the same logic as in the intial HTML request to figure "ok, so what was the the formatter used for this field in this entity in this view mode ?".

That's fragile, and currently the code actually fails to reproduce the exact same "view mode --> formatter" logic that happens in EntityRenderController::viewMultiple(). But the view_mode itself is irrelevant here, it's only used to retrieve the formatter.

So, much more robust: at field_preprocess time, we do know the plugin_id of the formatter being used to render the field, we can just put that info in the data-edit-id directly. No need to reproduce some complex logic to retrieve it again in the ajax request.

WOOT! Thanks, yched! Much appreciated! I was working on this myself, but failing (I was using entity_get_display()->getComponents() and was not getting any results :/).

The patch in #14 should be sufficient, because if the setting marks a field to be hidden from display, then the no metadata will be requested for it either.

And yes, Edit really needs more tests. I'm working on a test for Edit's metadata aspect right now.

In #17, I crossposted.

What you're saying in #16 makes sense, at least on first sight. I'd love to make it simpler, of course :) However, I don't see why the "view mode --> formatter" logic is fragile?

That being said, on second sight, your proposal is insufficient, because the primary reason for choosing to send the "view mode" to the client side is because that is necessary to re-render the field using field_view_field() once a field has been successfully modified through in-place editing. And that function needs $view_mode. You didn't modify that part (EditController.php/fieldForm()) in #16, and because of your other changes in #16, it'd be impossible to pass the view mode to it.

Initial unit tests, that would have caused this API change and resulting breakage to be detected by the testbot.

Note that the test can be cleaned up further. Also note that I'm using a mocked access checker because the entity type that the field_test unit tests use is *not* "node", and the Entity Access API for non-nodes is not yet ready, heck, even Node.module doesn't even implement it (#1862750: Implement entity access API for nodes).

@Wim Leers: the problem is that we're reproducing / duplicating the logic contained in EntityRenderController to route "view mode" to "a formatter used for a field in an entity". And that logic is not trivial and might change (see the discussion in #1875970-11: Pass EntityDisplay objects to the whole entity_view() callstack about displays per entity), so having it in several places for "recompute the formatter to get the metadata" & "recompute the formatter to redisplay the field" is less than ideal.

But ack for the need for view mode. Then yes, #14 or #15 is what we want for now.
I'll try to see if I can think of a way to streamline that.

#20: I agree that it's less than ideal, but for me personally (not being an expert in Field API internals, nor knowledgeable with the envisioned target architecture), it's hard to see a clean solution in the current state of the code base. If you say that field_view_field() will accept a formatter, then I'd be happy to get rid of all references to $view_mode! :)

Until that is possible, let us please focus on the bug at hand here (which completely breaks Edit!). I don't want to have both the view mode and formatter type in the field annotations. It'd require (a lot of) changes in the JS too. Let's make that change once we can apply it consistently.

In #19, I said that the tests could be cleaned up further. Well, I've done that now. There's now an EditTestBase class for the common parts.

Odd. I'll reroll tomorrow.

Had to reroll for testing something else, and I'm not sure this is 100% accurate since I can't run the tests right now, but it does fix the issue. Hope you don't mind. ;)

re @Wim #22
Sorry if my #20 was not clear. But sure, #14 is the correct fix, and it's what we should get in.

I'll try to see if I can come with a way to make the three code paths (initial rendering, metadata ajax callback, post-edit field re-rendering) less separate, but that's for another issue.
It's actually not that bad, the three code paths all rely on entity_get_render_display($entity, $view_mode) (= give me the display options to use when rendering this entity in this view mode). It's just that this function might change a bit in the future to allow more flexibility for contrib, and I need to keep the Edit.module use cases in mind.

If you say that field_view_field() will accept a formatter, then I'd be happy to get rid of all references to $view_mode! :)

field_view_field() *does* accept an array of display options (basically a formatter plugin_id + a couple other params including an array of formatter settings) instead of a view mode. But this won't work for your case - you'd need the whole display options array serialized in the data-edit-id string, not just the formatter plugin id. So that's a no go, it has to be field_view_field($view_mode).

#25: Of course I don't mind — thanks :)

#28: Thanks for the confirmation! And yes, I know that field_view_field() accepts an array of display options, but as you already say, that's not the same as a simple formatter plug-in id.

The problem was that I was generating this patch on a system with already other patches committed (locally). My bad. Nothing changed functionally between #22, #25 and this one, except I expect this one to be green. I'm hoping that yched can RTBC It.

#29: edit_metadata_fix_plus_tests-1876992-29.patch queued for re-testing.

+++ b/core/modules/edit/lib/Drupal/edit/MetadataGenerator.phpundefined
@@ -9,6 +9,7 @@
+use Drupal\entity\Plugin\Core\Entity\EntityDisplay;

My bad, but this "use" statement is actually not needed.
Other than that, tests look fine, this should be RTBC.

28 days to next Drupal core point release.

Oops, status change was unintentional, but - well, there you go :-)

Looks good to me and it effectively brings back the edit button.

Remove the use statement and changed a comment (80 chars violation, unless that's allowed ?)

#32: oh, heh, right! :)
#34: good catch.

This one is good to go!

Thanks for the fast turnaround on this, folks! Another major bug bites the dust. :)

Committed and pushed to 8.x. Thanks!