Remove group @legacy from jsonapi tests and fix deprecation messages

Thanks! That's not much, fortunately :)

This would be an excellent novice issue.

Working on this on DripalCon with jefftrnr.

Here is the patch for the first deprecated error. Providing as a separated patch for an easier review since we are changing the logic by removing the conditional "if" and just applying the suggested fix to the field value.

Re-uploading the patch to fix the failing test.

Replacing deprecated code at SeattleDrupalCon, this patch adds replacements for getUsername(), combined with patch #6

Adding fixes for "tests/src/Functional/ResourceTestBase.php" file to the existing patch.

Added replaced ftn for entityManager() to patch #9

Replaced final deprecated function along with patch #10

Thanks for working on this!

This needs to be committed to core first, afterwards I’ll port it back to the contrib module. Could you reroll this as a core patch? Thanks! :)

Rerolled

Looks good after applying #15

1. +++ b/core/modules/jsonapi/src/JsonApiResource/ResourceObject.php
   @@ -284,8 +284,9 @@ protected static function extractContentEntityFields(ResourceType $resource_type
   -    if ($entity_type->hasLabelCallback()) {
   -      $fields[static::getLabelFieldName($entity)]->value = $entity->label();
   +    $entity_label = $entity->label();
   +    if ($entity_label) {
   +      $fields[static::getLabelFieldName($entity)]->value = $entity_label;
        }
   

   This is a significant change; this calls $entity->label() always. There is no change record for this. #2495301: Deprecate user_format_name() and the label_callback for 9.x (not 8.x) introduced it. #2450793: Properly deprecate support for entity type label callbacks exists to apply this change all across core. That one seems to have a draft change record associated with it that #2495301 should have had and should have published: https://www.drupal.org/node/2481845.

   I think this is the correct change.

2. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
   @@ -70,7 +70,7 @@ public function testBundleSpecificTargetEntityTypeFromIssue2953207() {
   -        $user->getUsername(),
   +        $user->getAccountName(),
   

   This change is repeated many dozens of times, and is definitely fine :)

3. +++ b/core/modules/jsonapi/tests/src/Kernel/Controller/EntityResourceTest.php
   @@ -422,7 +422,8 @@ public function testCreateIndividual() {
   -    $this->assertTrue(entity_load_multiple_by_properties('node', ['uuid' => $response->getResponseData()->getData()->getIterator()->offsetGet(0)->getId()]));
   +    $entity_type_manager = $this->container->get('entity_type.manager');
   +    $this->assertTrue($entity_type_manager->getStorage('node')->loadByProperties(['uuid' => $response->getResponseData()->getData()->getIterator()->offsetGet(0)->getId()]));
   

   👍

Thanks to all of you who contributed to this patch! 🙏

Indeed, thank you!

I'm not so sure about that entity label change. Having an entity label field and having a callback aren't mutually exclusive.

The deprecated label callback means only one thing. That entity types that do have a dynamic label need to override the label method instead of defining a callback. Many entity types implement it currently and wouldn't have to, but one example would be Media::label(), which has a fallback if nothing is defined yet, we do something similar in TMGMT. getLabelCallback() was never supposed to be a public API, it's just an internal helper for dynamic labels, it predates entity classes, it was necessary in 7.x days when $user was a \stdClass.

I'm not exactly sure what \Drupal\jsonapi\JsonApiResource\ResourceObject::extractContentEntityFields() is trying to do. One thing it doesn't seem to properly handle is what happens if static::getLabelFieldName($entity) returns NULL, which is perfectly valid, the label is is optional. So maybe checking if what it returns actually is a field and exists in $fields would be a better condition?

But even then, it does other things like hardcoding ->value, there's no guarantee that that field actually has such a property.

I had doubts, but Berdir writing I'm not so sure about that entity label change. makes it pretty clear we want to look deeper into #19.1. Thanks @Berdir! 🙏

The deprecated label callback means only one thing. That entity types that do have a dynamic label need to override the label method instead of defining a callback.

Okay, so that method does have a @deprecated annotation, but it actually does not mean "this method is deprecated", it means "this method is deprecated for certain reasons for calling it"?

Based on #21, it sounds like we'd be better off committing this without #19.1. It seems like we should wait for #2450793: Properly deprecate support for entity type label callbacks to be completed before doing this?

+1 on doing a dedicated issue for that entity label stuff because it is not simple deprecation replacement, because as I wrote, even if it wouldn't be deprecated (which it is, for all cases), it should never have been used an API, it doesn't mean anything. Core will remove its label callbacks and then whatever it tries to do will change.

I suggest you create an issue, describe what exactly you are trying to solve there (because I still don't understand it) and then I can help come up with a better implementation. It seems completely inversed to me right now, as having a label callback often means that there is *no* label entity key, and it only works for user because that is hardcoded.

I noticed other per-module deprecation issues too for core and I think they are problematic anyway. We already closed another one as a duplicate. Most "pseudo-deprecated" API's already have dedicated issues, which add @trigger_error() calls to enforce having no left-over calls in core and these issues tend to be easier to review and commit, even though they can be considerably larger. This process makes much more sense for contrib modules.

Alright, thanks!

So let's:

• Update the patch to remove the label callback change
• Create a follow-up issue specifically to re-assess JSON:API's use of the label callback. (@gabesullice, could you do that?)

Btw, my patch in #2450793: Properly deprecate support for entity type label callbacks makes it fail with the current logic, which shows quite well what I meant. getLabelCallback() is and never was an API, having one or not doesn't mean anything, it's an internal helper. So I guess it makes sense to postpone that on the follow-up issue that we need to create.

My current thought on that this feels like logic that was specifically added for user entities, in a kinda-generic way that maybe doesn't need to be generic. And maybe it doesn't need to exist at all. Try combining it with the name module or a custom name alter hook. If we send back the (account) name field with the altered display name, than that results in overwriting that. A typical use case is sites generating a technical account name or using the e-mail and having one or multiple fields for the displayed name. Loading and saving a user entity through jsonapi. would then overwrite the account name with the dynamic display name, which is not correct*. At the same time, you do want to make the display name accessible too, but that should then be in a separate computed thing that isn't saved back.

* Didn't try this, but would be good to test, see \Drupal\Tests\user\Kernel\UserEntityLabelCallbackTest::testLabelCallback().

My current thought on that this feels like logic that was specifically added for user entities, in a kinda-generic way

I am certain that this thought is accurate :)

This issue overlaps with one I just opened, #3052308: Remove deprecated code and @group legacy annotation from JSON:API tests. Some of this code is used in tests which are marked @group legacy and should not be legacy tests.

Closed #3054403: Fix use of deprecated code in Jsonapi module as a duplicate.

As suggested in #3052308: Remove deprecated code and @group legacy annotation from JSON:API tests, I'm bringing these two tests together, removing @group legacy kind of gives us test coverage for this.

For starters, here is a patch with just those removals. That does result in one extra deprecation:

23x: Fetching the "jsonapi.entity_access_checker" private service or alias is deprecated since Symfony 3.4 and will fail in 4.0. Make it public instead.

The 4.0 fail thing makes me wonder whether this already came up in the issues that test updating. Will look into that.

So, the problem is fairly obvious, \Drupal\Tests\jsonapi\Kernel\Controller\EntityResourceTest::createEntityResource() accesses the private service jsonapi.entity_access_checker, pretty sure that Wim isn't going to like making that public, but inlining the whole definition seems pretty ugly, especially due to the optional setter injections there.

Also reverted the label callback thing, I might take care of that in the issue that is removing its usage.

Updated #31 patch to meet drupal-check check

Updated patch #34 based on test results

I removed the change in ResourceObject on purpose, as discussed above, we'll deal with that elsewhere.

Updated #31 based on comments @Berdir #31, #38

+++ b/core/modules/jsonapi/jsonapi.services.yml
@@ -130,7 +130,6 @@ services:
-    public: false

This is the one thing I don't like, as predicted by @Berdir in #31 :)

I would frankly prefer deleting \Drupal\Tests\jsonapi\Kernel\Controller\EntityResourceTest altogether over making this private service public. EntityResourceTest served a purpose originally, when it was added in #2726265: [BUGFIX] Add test coverage for EntityResource, Routes and ResourceManager (exactly three years ago tomorrow), but it doesn't anymore today. It is too coupled to internals. Our functional test coverage tests a vast superset of what this tests. So I say: just delete it if it gets in the way.

@e0ipso & @gabesullice: What do you think?

Can't comment on whether there is anything worth keeping of that test, but one thing we could try is to just get that from the service.. maybe there is no reason to create that on our own, and if ther really is (injecting things changing or something) then maybe that is either a bug (services shouldn't really have state, in theory anyway) or we can work around it in the test by unsetting the service on the container and initializing it again.

Yes, this seems to work fine, not sure why we didn't do that before? It is a controller, so maybe this was changed later on and wasn't a service initially?

Also one usage of entity manager was apparently fixed elsewhere.

#3057175: Implementation of user name in JSON:API can result in overwriting data

Spotted two simple things, and I think this is ready after those are addressed.

1. An additional use of getUsername() was recently added in #3053827: Leaked cache metadata detected when using JSON:API to GET a threaded comment when RDF module is installed (a day after the most recent patch) that needs to be addressed, in Drupal\Tests\jsonapi\Functional\JsonApiRegressionTest::testLeakedCacheMetadataViaRdfFromIssue3053827
2. I recommend adding a followup issue for removing \Drupal\Tests\jsonapi\Kernel\Controller\EntityResourceTest, largely because then we can add a @todo that references the issue.That way, if future issues run into problems with EntityResourceTest, contributors will know that the usefulness of these tests is no longer certain, and they can decide if their time is better spent helping remove EntityResourceTest vs refactoring so it passes with their changes.

Can we also file a follow-up to remove the @see to issues and from the test names and rename JsonApiRegressionTest - the @see's are largely pointless - git blame exists - and it's not a regression test and the fact that some is regression is not necessary anyway to encode into the name of a test.

#46:

Can we also file a follow-up to remove the @see to issues and from the test names and rename JsonApiRegressionTest - the @see's are largely pointless - git blame exists - and it's not a regression test and the fact that some is regression is not necessary anyway to encode into the name of a test.

git blame is unfortunately pointless at this point, because we lost JSON:API's commit history when bringing it into Drupal core.

Furthermore, the point of this test is to have a single location where we add test coverage for highly specific (entity type-specific, field-specific, configuration-specific …) bugs, and they all follow the same pattern. All the generalizable test coverage lives in \Drupal\Tests\jsonapi\Functional\ResourceTestBase.

So I'd rather keep it as-is. Is it a common pattern? No. Is it a productivity booster? Yes.

#47: That addressed #45.1 👍

This addresses #45.2 and doing what I said in #41. This issue is about fixing meta aspects of JSON:API tests, this is related to that, so it seems acceptable to me to be doing this in one go.

@Wim Leers the problem is that going forward the pattern of @see to the issue and nid in the test method is unnecessary and yet another thing for the reviewer to check and going forward it is not necessary. Also the test is not a regression test. But anyhows.

I'm surprised that we remove a test in #48 because that could have been a follow-up. We stopped changing the service to public in #43 anyway. So rather than commit #48 I'm going to commit #47 because the work to guarantee that

It is too coupled to internals. Our functional test coverage tests a vast superset of what this tests. So I say: just delete it if it gets in the way.

has not been done here and moving forward on not using deprecated code in tests is more important.

Committed 6d3ac6c and pushed to 8.8.x. Thanks!

Created #3060836: Remove 99% of \Drupal\Tests\jsonapi\Kernel\Controller\EntityResourceTest since it is a subset of the functional tests