Move jsonapi constants to an enum and deprecate the 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?

Honestly I'm not super familiar with what you mean, if you can point to an example I'm happy to modify this approach.

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.

Yeah it's used in contrib, what benefit does the value object have over the enum?

Yeah I think Enum is the proper approach here, these values are constant and don't change so a value object doesn't do anything.

Possible to add small deprecation test.

Deprecation policy explicitly excludes adding tests to show deprecation message.

3.2 https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... →

If you meant something else let me know.

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.

Yeah those other issues should not have tests for the deprecation specifically.

Here is another example: https://www.drupal.org/project/drupal/issues/3490222#comment-15876497 🐛 Reinstate drupal_common_theme() and deprecate it Active

It makes more work for the committers because they remove them.

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

Sorry, that wasn't what I meant. I meant core commiters try to remove those types of tests before committing them, so adding it here is extra work for them to catch and work to do when committing them.

The policy is that tests looking for a deprecation only should not exist. Maybe that page needs some clarification to be more explicit.

Compare the MR https://git.drupalcode.org/project/drupal/-/merge_requests/10364/diffs

With the actual commit: https://git.drupalcode.org/project/drupal/-/commit/e2b71bfe47e789fda4bc4...

@mstrelan, you meant a value object for that array, not for the enum right? I think I misunderstood your question.

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.

I think that is out of scope for this issue, it would involve also changing how the hook_jsonapi_entity_filter_access and hook_jsonapi_ENTITY_TYPE_filter_access are actually invoked where as this is just a shift over to an enum to allow deleting jsonapi.module in 12.x I'm happy to open a follow up to investigate that change.

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

Where can you put the constants that will auto load like a .module file?

Making them an enum allows contrib or custom code using them to get it with a use statement.

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.

Yes, the difference in both those cases is that these constants are in a .module file which means they are available 100% of the time as long as you have the module enabled. We don't know how contrib or custom code used the constants.

If we are OK with saying these constants are only available during the time that a given interface or the temporary guard is loaded then I'm ok with moving them, it's definitely easier.

I did it this way so that no matter how people are using these constants they can add the enum and continue using them.

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;

Not sure why I was missing that point, that is way more straightforward.

I will change this to a class instead of an enum.

Then update the references, comments and messages.

ok that's been done, I'll keep an eye on tests.

Sorry for the runaround, it just was not clicking for some reason! That is way cleaner this should be good to review now.

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

Maybe, I did look at match, I disagree it's clearer, but forgot to reply.

The other one feels out of scope, but if it's going to happen, it should be here so I'm not quite sure what to do.

I'll take another look and reply to the 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.

I added comments directly to the mr of you guys think this is enough to rtbc I can open the follow up for the 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?

@ptmkenny, I think that's a great question, I do want to point out this is not an enum though.

@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.

Rebased

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: 📌 Defer disruptive 11.3 deprecations for removal until 13.0 Active

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:

   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!

xjm → closed merge request !10652

Belatedly publishing the CR, oops.