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