Deprecate JSON:API filter constants moving them to class constants

I didn't look too thoroughly, but ideally we can pass around the enum itself rather than the string backed values. Is that possible or are there too many bits to untangle?

I take it back, since these are used as array keys you need to use the scalar value.

The array structure is a bit weird though, maybe it would make sense to have a value object class instead of associative arrays. Maybe enum doesn't make sense after all. Let's see what others think?

The notorius temporary class.

I kinda agree that the whole enum dance doesn't really add much. If we are to change this a value object could make sense, or, since this is really only used in this class, perhaps just have class constants with limited visibility? Then we also scope it into this class where it is used, and then can clean up the .module file?

It is used in contrib though: https://git.drupalcode.org/search?group_id=2&scope=blobs&search=-path%3A...

so Limited visibility might not be a good idea.

Possible to add small deprecation test.

We add deprecation tests for a lot of other deprecations should would say this probably would need that.

Will leave for others in the review queue to take a look though.

We removed tons of deprecation tests for D11 wasn’t a problem

Yeah for the array. So the constants would essentially become object properties. I've been AFK and will continue to be for another week or so, so apologies if I'm talking nonsense, as I'm just glancing at it.

IMHO for a quick fix they should stay as constants instead of enum. Enum is better when a variable can be one of the enum case values, but these are only ever array keys as far as i know

There are some NodeInterface constants, is that loaded differently?

class TemporaryQueryGuard {

  public const JSONAPI_FILTER_AMONG_ENABLED = 'filter_among_enabled';

...
}

That's the class constant route i talked about earlier.

I'm failing to understand how they wouldn't be available. Can't contrib do this?

<?php

use Drupal\whatever\SoneInterface;

$foo = SomeInterface::MY_CONSTANT;

Should this be moved to NW for the feedback for the 2 threads?

I agree with @Berdir's comments about the value object, but in the interest of removing the .module file I think what you have now is the simplest. We should open a follow up to move to a value object.

Some of the discussion concerned whether this should be made internal or available to contrib/custom code; absolutely these constants need to be available for contrib + custom code.

One of the key uses of these constants is to enable JSON:API filtering in custom entities. For example:

  /**
   * Implements hook_jsonapi_ENTITY_TYPE_filter_access() for entity_type.
   */
  #[Hook('jsonapi_entity_type_filter_access')]
  public function jsonapiEntityTypeFilterAccess(EntityTypeInterface $entity_type, AccountInterface $account) : array {
    return [\JSONAPI_FILTER_AMONG_ALL => AccessResult::allowed()];
  }

With the MR, this would become:

    return [JsonApiFilter::JSONAPI_FILTER_AMONG_ALL => AccessResult::allowed()];

And everything would continue to work as before, which is great.

However, I wonder if there is a policy for naming these enum cases. With JsonApiFilter::JSONAPI_FILTER_AMONG_ALL, we have JsonApiFilter as the enum name and JSONAPI_FILTER as part of the case name. We could eliminate the repetition by renaming it something like JsonApiFilter::AMONG_ALL.

Also, core uses all caps for constants like JSONAPI_FILTER_AMONG_ALL, but is this appropriate/the policy for enums?

@nicxvan Ah, I see it was proposed as an enum but got changed to a class. Thanks for the clarification.

I think this is good, and helpful to get the last bit out of the module file. Although there are perhaps ways to improve this class even more, this would mean more changes, and bigger impact. This is still a temporary (ha! years) class. Lets isolate this to the existing classes and change as little as possible to make the change possible.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Seems nicely rebased, back to RTBC

Sorry, the title, and thus the commit message, do not match what is in the MR.

Did some CR/IS/title updates to make sure all is consistent.

Added a question and a nitpick.

Made the few small changes you asked for, setting RTBC, all seem straightforwards enough and tests are still green. Updating Cr right now.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased

My first thought on looking at this issue title was, "That needs subsystem signoff." Then I see @bbrala has RTBCed it -- perfect!

Crediting @smustgrave for issue management, @ptmkenny for feedback on visibility, and @longwave for committer review.

I did a full code review, including carefully reading everything to ensure 1:1 replacements of the API, the usages, and the docs. It looks great! The only two issues I found were that the deprecations incorrectly link the issue instead of the CR, and that it is deprecating these for removal in Drupal 12.

That latter part was correct up until recently, when we decided to start deferring deprecations for APIs contrib might want to support to Drupal 13. (We only just agreed on the change and are drafting an announcement.) Based on @ptmkenny's comment, contrib and custom code does indeed interact with these constants, so it might affect the ability of code to support both 10.5 and 12.0 at the same time if we removed these in the 12.x branch before 10.5 is EOL. This is kind of a borderline case, but let's go ahead and enact the policy.

Reference: #3518671: [policy, no patch] Defer disruptive 11.3 deprecations for removal until 13.0

So, let's correct those two things in the MR. I left suggestions for the second. Whoever fixes them can just set the issue directly back to RTBC since they are very small.

Thanks!

Updated the versions to be deprecated in 11.3 and removed in 13 also updated the CR

1. ✅ Checked the new CR references. ll match the CR and are formatted correctly.
2. ✅ Applied locally and reviewed with:
   

   git diff --staged --color-words="."
   

   ...which led to me muttering:
   son pi ilter::!

3. 🤦‍♀️ Had a brief moment of "but wait! data typing!" but these are of course constants and cannot vary, so the syntax of typing them is not supported at all. Silly release manager.
4. ✅ Checked for any remaining core usages of the deprecated constants:

   [ayrton:maintainer | Sun 13:12:06] $ grep -r "JSONAPI_FILTER" * | grep -v "vendor" | grep -v "node_modules" | grep -v "phpstan-tmp"
   core/modules/jsonapi/jsonapi.module:const JSONAPI_FILTER_AMONG_ALL = 'filter_among_all';
   core/modules/jsonapi/jsonapi.module:const JSONAPI_FILTER_AMONG_PUBLISHED = 'filter_among_published';
   core/modules/jsonapi/jsonapi.module:const JSONAPI_FILTER_AMONG_ENABLED = 'filter_among_enabled';
   core/modules/jsonapi/jsonapi.module:const JSONAPI_FILTER_AMONG_OWN = 'filter_among_own';
   

5. ✅ I thought about the testing gate. According to our deprecation policy:

   A test is only required when there is backwards compatibility (BC) logic that needs to be tested.

   And:

   Do not add a test [...] that the deprecation error is triggered.

   Thus, no test coverage is required for this deprecation, since it is one of the simplest possible kinds of deprecations.

So, committed to 11.x! Thanks all!

Belatedly publishing the CR, oops.

<3