Problem/Motivation

With JSON:API allowing to disable fields through an event subscribe an application could have a (contrib) module that disables a field in a resource. If you want to enable that field again in your resource responses this currently isn't possible.

Steps to reproduce

Add an event subscriber that disabled a field. There is now no way to re-enable that field without any weird magic.


namespace Drupal\jsonapi_test_resource_type_building\EventSubscriber;

use Drupal\jsonapi\ResourceType\ResourceTypeBuildEvents;
use Drupal\jsonapi\ResourceType\ResourceTypeBuildEvent;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

/**
 * Event subscriber which tests disabling resource types.
 *
 * @internal
 */
class ResourceTypeBuildEventSubscriber implements EventSubscriberInterface {

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents(): array {
    return [
      ResourceTypeBuildEvents::BUILD => [
        ['disableResourceTypeFields'],
      ],
    ];
  }

  /**
   * Disables any resource type fields that have been aliased by a test.
   *
   * @param \Drupal\jsonapi\ResourceType\ResourceTypeBuildEvent $event
   *   The build event.
   */
  public function disableResourceTypeFields(ResourceTypeBuildEvent $event) {
    foreach ($event->getFields() as $field) {
      // Disable the internal Drupal identifiers.
      if (str_starts_with($field->getPublicName(), 'drupal_internal__')) {
        $event->disableField($field);
      }
    }
  }

}

Proposed resolution

Propose we add the possibility to enable fields again in the ResourceTypeBuildEvents.

Remaining tasks

  1. Implement the proposed resolution by adding enableField to the EventSubscriber and enabled to ResourceTypes.
  2. Write tests
  3. Add change record
  4. Follow up to update documentation

User interface changes

n.a.

API changes

New method enableField on ResourceTypeBuildEvents and new method enabled on ResourceType to modify the resource type.

Data model changes

n.a.

Release notes snippet

n.a.

Issue fork drupal-3110831

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mglaman created an issue. See original summary.

xjm’s picture

Version: 9.0.x-dev » 9.1.x-dev

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bbrala’s picture

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bbrala’s picture

Status: Active » Needs review

This annoyed me so spent a little time implementing. Implementation was ealy enough.

bbrala’s picture

Title: Cannot un-disable a resource type field disabled by a previous ResourceTypeBuildEvent subscriber » Un-disable a resource type field disabled by a previous ResourceTypeBuildEvent subscriber
Issue summary: View changes
Issue tags: +Needs change record
bbrala’s picture

Title: Un-disable a resource type field disabled by a previous ResourceTypeBuildEvent subscriber » Method to enable a resource type field disabled by a previous ResourceTypeBuildEvent subscriber

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

wim leers’s picture

Issue tags: +API addition

Once there's a change record, this is insta-RTBC IMHO?

bbrala’s picture

I would say, if you have looked at the code, I can easily write a CR tomorrow.

wim leers’s picture

Status: Needs review » Needs work

Can you write the CR and address the nits? Then this is RTBC 🤓

z3cka’s picture

I took a stab at writing a change record for this: https://www.drupal.org/node/3319533 Please, any feedback welcome.

I'm going to go ahead and test the MR in my current project, as this is just what I need right now. Cheers!

z3cka’s picture

Issue tags: -Needs change record
bbrala’s picture

Assigned: Unassigned » bbrala

Thanks for CR and the testing. I've tweaked the CR a little so its a bit more expressive and has your example. I'll now work on the few nits that @Wim Leers has mentioned.

bbrala’s picture

Status: Needs work » Needs review

I've addressed all the little nits. Also retargeted to 10.1.x and rebased. Thanks everyone. Setting back to NR :)

bbrala’s picture

Assigned: bbrala » Unassigned
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Verified the test coverage running locally without the fix. I got

Error : Call to undefined method Drupal\jsonapi\ResourceType\ResourceTypeBuildEvent::enableField()

Which is good!

All threads are resolved on the MR
All tests are green
Change record is added and make sense (to me at least)
New functions are typehinted

This looks good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

There are several other options here. You could alter the service that subscribes the event or you could implement an event subscriber with a higher priority and stop propagation. I'm not sure that adding enableField() to fight it out with disableField() is going to improve the situation. I can imagine people adding an event to disable a field, that was disabled and then enabled. Feels odd.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Alright issue should be reworked based on #22 or explained why this is the better approach.

Decision should be documented in issue summary please

Thanks.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

matthand’s picture

Is there a way to re-enable the UUID field that is disabled on node resources by JSON:API without this new enableField method? I don't think so. Real world scenario of trying to match a legacy Drupal 8 JSON:API endpoint to prevent consumer apps requiring a rebuild.

z3cka’s picture

I agree that this can probably be handled better, but until a better solution is provided, I went ahead and rebased the temp fix on to the Drupal 10.3.x branch for those that need a compatible patch.

bbrala’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

I really think this is the way, it might need some priority juggling to make work in your application but that should be fine. I'll update the MR to 11.x and make sure codestyle and everything is up to date.

bbrala’s picture

Status: Needs work » Needs review

Moved all changed to 11.x, also updates a return type that didnt make sense. I think this is good like this. I've updated the issue summary also.

I still think this is the best option for adjusting, even though this could mean you enable and disable multiple times. But i don't think this will be an actually issue in implementations.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Reading the CR and it appears to contain good detail with a nice example

Test-only feature has already been ran

1) Drupal\Tests\jsonapi\Kernel\ResourceType\ResourceTypeRepositoryTest::testResourceTypeFieldEnabling
Error: Call to undefined method Drupal\jsonapi\ResourceType\ResourceTypeBuildEvent::enableField()
/builds/issue/drupal-3110831/core/modules/jsonapi/tests/modules/jsonapi_test_resource_type_building/src/EventSubscriber/LateResourceTypeBuildEventSubscriber.php:41
/builds/issue/drupal-3110831/vendor/symfony/event-dispatcher/EventDispatcher.php:206
/builds/issue/drupal-3110831/vendor/symfony/event-dispatcher/EventDispatcher.php:56
/builds/issue/drupal-3110831/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php:165
/builds/issue/drupal-3110831/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php:132
/builds/issue/drupal-3110831/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php:131
/builds/issue/drupal-3110831/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php:209
/builds/issue/drupal-3110831/core/modules/jsonapi/tests/src/Kernel/ResourceType/ResourceTypeRepositoryTest.php:246
ERRORS!
Tests: 13, Assertions: 192, Errors: 1.

So coverage is there.

Added some super nitpicky :void returns directly.

Believe all feedback has been addressed.

  • longwave committed 257fb3b3 on 10.4.x
    Issue #3110831 by bbrala, z3cka, smustgrave, mglaman, wim leers,...

  • longwave committed a1c6ae78 on 11.x
    Issue #3110831 by bbrala, z3cka, smustgrave, mglaman, wim leers,...
longwave’s picture

Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to 11.x for 11.1.0 and backported to 10.4.x as a minor API addition to keep things in sync. Also published the change record.

Committed and pushed a1c6ae78c3 to 11.x and 257fb3b34b to 10.4.x. Thanks!

wim leers’s picture

bbrala’s picture

We'll get there, step to step.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.