field_language() always NULL for non-configurable fields (=> in-place editing of node title fails)

Adding D8MI tags.

Looking into this.

All right, the problem is the field_language() function (which is deprecated, but not clear in favor of what) will not return any value for non-configurable fields.

So Drupal\edit\EditController::fieldForm() uses the (not yet deprecated) field_view_field() which passes on the langcode given to (the deprecated) field_language(). That will see that a concrete field was passed in and would only return a non-NULL value if it was a configurable field. So for non-configurable fields, the field language would always come down to NULL. That is not very useful for rendering it :D Since non-configurable fields behave the same way for rendering that configurable fields are, I think we should use the same logic. This definitely makes it flawlessly work :) Expert entity API advise would be welcome nonetheless :) The fix is just one line.

Looking into providing test coverage by - I guess - duplicating what we have for body.

Hm, yeah - field_language() is totally not prepared to be called on base fields...

We really need to get rid of those field_language() calls in field_view_field(), and consider that callers of field_view_field() pass the $entity in the correct language.

Shouldn't be too complex, but probably better done as part of #2134887: move field_view_field() / field_view_value() to methods.
Meanwhile, if the patch fixes IPE of node title, it's fine by me.

Doh, the removal of return $display_langcodes; was not intended. This is really a one line patch :D

+++ b/core/modules/field/field.deprecated.inc
@@ -906,7 +906,7 @@ function field_language(EntityInterface $entity, $field_name = NULL, $langcode =
-  elseif ($definitions[$field_name]->isFieldConfigurable()) {

Not sure whether this will work with non-configurable fields but I guess we should at least match this change in the if-branch.

I confirmed with manual testing that the bug as described exists in HEAD and is fixed by #10. This updated patch addresses #12. Given the new Entity Translation API, I see no reason to keep field_language() caring about isConfigurable(), so +1 to this fix.

I think all we need now is to enhance our functional tests in edit module. We have existing functional tests there that test a configurable field; we should add a nonconfigurable field to those tests as well. Such a test should fail without this patch and pass with it.

Re #4: that's now in a separate issue: #2149933: Improve Edit module JS to use .find(...) instead of .is(':has(...)')

Good to know that #13 passes current tests, but back to "needs work" for adding a test, and setting component to edit.module since that's where the test is needed.

Added tests modeled after the body tests. Retrieve the node title edit, ensure the JSON returned is proper, edit the title, submit the edit, ensure it is not yet saved, save, ensure it is saved. This should be a full circle test. It fails for me without the fix and passes with the fix too. :) Nothing changed in the actual patch, so the test only patch is also the interdiff :)

Thanks!

(the tests in #16 are done running, not sure why that hasn't been reported back to the issue, but they pass and fail as expected).

Awesome!

I was super bummed to see that node title editing didn't work despite all of our efforts. Confirmed this fixes the bug (although the title field is hard to hit atm). Great to see an RTBC patch for this *with* tests. :D

Committed and pushed to 8.x. Thanks!

Hm. Actually, something's up...

$ git pull --rebase
remote: Counting objects: 949, done.
remote: Compressing objects: 100% (124/124), done.
remote: Total 543 (delta 306), reused 493 (delta 277)
Receiving objects: 100% (543/543), 115.76 KiB, done.
Resolving deltas: 100% (306/306), completed with 269 local objects.
From git.drupal.org:project/drupal
   1a80341..560e5af  8.x        -> origin/8.x
First, rewinding head to replay your work on top of it...
Applying: Issue #2148797 by Gábor Hojtsy, effulgentsia: Field_language() always NULL for non-configurable fields (=> in-place editing of node title fails).
Using index info to reconstruct a base tree...
M	core/modules/field/field.deprecated.inc
Falling back to patching base and 3-way merge...
Auto-merging core/modules/field/field.deprecated.inc
CONFLICT (content): Merge conflict in core/modules/field/field.deprecated.inc
Failed to merge in the changes.
Patch failed at 0001 Issue #2148797 by Gábor Hojtsy, effulgentsia: Field_language() always NULL for non-configurable fields (=> in-place editing of node title fails).
The copy of the patch that failed is found in:
   /Users/webchick/Sites/8.x/.git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

Must be conflicting with something else that was committed today. Sending for a re-test.

16: edit-title-fix-16.patch queued for re-testing.

I would, however, like to get this in by alpha7 so we can show this new capability off, so tagging for that.

Method names in removed hunks have changed in #2143263: Remove "Field" prefix from FieldDefinitionInterface methods.

Thanks for the reroll! Let's get this in! :)

Committed/pushed to 8.x, thanks!

Great, thanks!