ResourceTypes should know about their fields

Thanks for laying out the proposal so clearly in the issue summary! I was able to instantly grok where you were going with this :)

I like this a lot!

1. What's not yet clear to me is what the purpose is for NullResourceFieldDefinition?

2. +++ b/src/ResourceType/ResourceType.php
   @@ -131,6 +160,48 @@ class ResourceType {
   +  protected function createResourceField($field_name, $alias) {
   
   @@ -244,12 +315,14 @@ class ResourceType {
   +   * @param array $field_definitions
   +   *   The field definitions for this resource type.
   
   @@ -259,23 +332,41 @@ class ResourceType {
   +  public function __construct(EntityTypeInterface $entity_type, $bundle, $deserialization_target_class, array $field_definitions = [], $internal = FALSE, $is_locatable = TRUE, $is_mutable = TRUE, array $field_mapping = []) {
   

   I think that not passing Drupal's Field API's FieldDefinitionInterface objects into the ResourceType constructor (and then calling this protected createResourceField() for each of them), but instead passing the Resource(Attribute|Relationship)Definition value objects into the constructor would also solve the problem you highlighted:

   It lacks a good solution for config entities, given that they don't have field definitions.

   Because then the ResourceType object remains a simple, dumb value object. With no logic. With only metadata. The complexity would then continue to live in ResourceTypeRepository. Just like it already does for computing the relatable resource types, it could also contain this mapping logic, including that for config entity types.

Great first iteration! 👌 I'm excited to see where you'll take this! This feels like the next logical step after #2977669: Spec Compliance: some entity types have an "id", "type" or "uuid" field, this violates the spec.

I'm not sure this is a Feature Request, this seems more like a Task? It doesn't bring new features, it's about refactoring internals.

AFAICT this is a hard blocker for #3014283: Use ResourceTypeFields to statically determine the field normalizer?

👍 Looking forward to this getting in! 😀

🎉

Looks cool! I see how this will be necessary. I'm a little worried about config entities since they don't have fields and because it's impossible to figure out what properties exist without having an instance of that config entity in hand.

I'm reviewing this mostly because I want to see how it would interact with #3015438: Wrap entity objects in a ResourceObject which carries a ResourceType. And there's good news, it won't. This patch isn't removing or changing and of the methods available on the ResourceType object. That was even more of a pleasant surprise because I assumed this would need to refactor Routes.php.

I like the idea of giving the ResourceType object a richer understanding of its fields. Field mappings have become quite complex and we've shoved a lot of information into a simple indexed array and overloaded the meaning of NULL and FALSE in the field mapping to mean various things.

I think I feel the same as @Wim Leers does in #7 about not moving the field mapping stuff out of ResourceTypeRepository. What does that buy us? Are we missing something?

1. +++ b/src/ResourceType/fields/ResourceField.php
   @@ -0,0 +1,77 @@
   +  public function getDataDefinition() {
   +    return $this->dataDefinition;
   +  }
   

   The purpose of this is not clear to me.

2. +++ b/src/ResourceType/fields/ResourceField.php
   --- /dev/null
   +++ b/src/ResourceType/fields/ResourceFieldInterface.php
   

   Should this interface have isEnabled() too?

3. +++ b/src/ResourceType/fields/ResourceFieldInterface.php
   @@ -0,0 +1,27 @@
   +interface ResourceFieldInterface {
   ...
   +  public function getOriginalName();
   ...
   +  public function getAlias();
   

   Let's stick with the names we use everywhere else: "public" & "internal".

4. +++ b/src/ResourceType/fields/ResourceFieldInterface.php
   @@ -0,0 +1,27 @@
   +interface ResourceFieldInterface {
   
   +++ b/src/ResourceType/fields/ResourceRelationshipInterface.php
   @@ -0,0 +1,25 @@
   +interface ResourceRelationshipInterface {
   

   Why have interfaces for these? If these are pure value objects, it seems unnecessary.

5. +++ b/src/ResourceType/fields/ResourceFieldInterface.php
   @@ -0,0 +1,27 @@
   +  public function isMultiple();
   

   Would getCardinality be better here? Perhaps not, that's a Drupal thing. This will really only inform whether an array or NULL is appropriate when normalizing empty fields.

1. +++ b/src/ResourceType/fields/NullResourceField.php
   --- /dev/null
   +++ b/src/ResourceType/fields/ResourceField.php
   

   Ubernit: drop the fields directory.

2. +++ b/src/ResourceType/fields/ResourceField.php
   @@ -0,0 +1,77 @@
   +class ResourceField implements ResourceFieldInterface {
   

   Nit: I think a better name for this is just FieldType, mirroring ResourceType. "Resource" is already implied by its namespace.

3. +++ b/src/ResourceType/fields/ResourceRelationship.php
   @@ -0,0 +1,43 @@
   +  public function setRelatableResourceTypes(array $relatable_resource_types) {
   

   Could this be turned into withRelatableResourceTypes and return a new instance of itself, making the object immutable?

Rebased #4 on HEAD.

(I did #17's rebase to ensure that @gabesullice's analysis in #15 is still accurate: that they don't conflict. Happy to report this is still true! This even applies cleanly to both HEAD and HEAD+#3015438: Wrap entity objects in a ResourceObject which carries a ResourceType!)

1. +++ b/src/ResourceType/ResourceAttribute.php
   @@ -0,0 +1,91 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function isMultiple() {
   +    return $this->isMultiple;
   +  }
   

   I know you said you're not ready for naming nits, but I'll forget this if I don't say it now:

   The spec's language for this is "has-one" vs "has-many". So, maybe s/isMultiple/hasMany?

2. +++ b/src/ResourceType/ResourceFieldFactory.php
   @@ -0,0 +1,160 @@
   +  protected static function create($field_name, $alias, $is_reference) {
   

   ❤️ this will make it very easy to experiment in contrib by overriding this bit of the factory. I might go a step further and add createAttribute and createRelationship so that their operations could be more granularly tweaked.

3. +++ b/src/ResourceType/ResourceFieldFactory.php
   @@ -0,0 +1,160 @@
   +    static $field_definitions;
   +    $cid = sprintf('entity:%s:%s', $entity_type->id(), $bundle);
   

   This feels like a premature optimization and I think because of the performance/cacheing work we've already done WRT to the resource type repository it won't have much of an effect. The field manager already has its own caching anyway.

4. +++ b/src/ResourceType/ResourceFieldFactory.php
   @@ -0,0 +1,160 @@
   +    $resource_field = static::create($field_name, $alias, $is_reference);
   +    $resource_field->setIsMultiple($is_multiple);
   

   Why use a setter for cardinality and nothing else? This feels like it's an accommodation for some other module in the ecosystem. If that's case, could that module override the create method instead of using this setter?

5. +++ b/src/ResourceType/ResourceFieldInterface.php
   @@ -0,0 +1,43 @@
   +  /**
   +   * @return \Drupal\Core\TypedData\DataDefinitionInterface
   +   */
   +  public function getDataDefinition();
   ...
   +  /**
   +   * @param \Drupal\Core\TypedData\DataDefinitionInterface $data_definition
   +   *
   +   * @return mixed
   +   */
   +  public function setDataDefinition(DataDefinitionInterface $data_definition);
   

   I don't think that these methods should be part of this patch. I know that it's necessary for jsonapi_schema, but I don't think that's fleshed out enough to justify them. F.E. why shouldn't this be getSchema and setSchema? What other purpose would knowing the data definition have?

   Couldn't we extend/decorate this class and experiment with what this method should be in contrib thanks to the factory?

6. +++ b/src/ResourceType/ResourceType.php
   @@ -307,13 +379,12 @@ class ResourceType {
   -  public function __construct($entity_type_id, $bundle, $deserialization_target_class, $internal = FALSE, $is_locatable = TRUE, $is_mutable = TRUE, $is_versionable = FALSE, array $field_mapping = []) {
   -    $this->entityTypeId = $entity_type_id;
   +  public function __construct(EntityTypeInterface $entity_type, $bundle, $deserialization_target_class, array $field_definitions, $internal = FALSE, $is_locatable = TRUE, $is_mutable = TRUE, $is_versionable = FALSE) {
   +    $this->entityType = $entity_type;
   
   @@ -321,14 +392,33 @@ class ResourceType {
   +      $resource_field_factory = \Drupal::service('jsonapi.resource_field.factory');
   
   @@ -389,4 +488,103 @@ class ResourceType {
   +  protected function getAllFieldNames() {
   ...
   +  protected static function getFieldMapping(array $field_names, EntityTypeInterface $entity_type, $bundle) {
   

   I still think moving the field derivation into the ResourceType class seems icky. Perhaps you're concerned about adding even more bloat to the ResourceTypeRepository? If that's the case, maybe we need a ResourceTypeFactory too. I'd be open to that change as a precursor issue or followup.

7. --- /dev/null
   +++ b/src/ResourceType/ResourceTypeComputer.php
   

   What's this file for?

I'm definitely +1 to adding an object that'll capture more information about a resource type's fields.

I expected to see some changes/simplifications elsewhere in JSON:API thanks to this. For example, in EntityResource::addToRelationshipData we have all this that could be simplified:

  public function addToRelationshipData(ResourceType $resource_type, FieldableEntityInterface $entity, $related, Request $request) {
    $resource_identifiers = $this->deserialize($resource_type, $request, ResourceIdentifier::class, $related);
    $related = $resource_type->getInternalName($related);
    // According to the specification, you are only allowed to POST to a
    // relationship if it is a to-many relationship.
    /* @var \Drupal\Core\Field\EntityReferenceFieldItemListInterface $field_list */
    $field_list = $entity->{$related};
    /* @var \Drupal\field\Entity\FieldConfig $field_definition */
    $field_definition = $field_list->getFieldDefinition();
    $is_multiple = $field_definition->getFieldStorageDefinition()->isMultiple();

1. Touché ✅
2. I was actually thinking of having all three. Then an extender could override to affect either type, or just one, without caring about the logic. Minor suggestion, not a blocker.
3. 👍 ✅
4. 👍 ✅
5. I'm okay with the coupling happening outside of core for now. There's some renewed discussion about making JSON:API recognize JSON-Schema, at which point I think it'd be safe to move into core. ✅
6. I don't think changing the name helps much. It feels weird to me to have so much logic happening in the construction of the class. It also forces this static service call: $resource_field_factory = \Drupal::service('jsonapi.resource_field.factory');. It seems like a factory is designed to solve this problem exactly.
7. I figured, thanks for confirming :) ✅

This interdiff addresses items with a green checkmark above. I also went ahead and addressed some small nits in the interest of saving time, like renaming getAlias to getPublicFieldName to match the language used everywhere else.

@e0ipso, I think I might have come up with a compromise after your comment in #24.6. This moves the logic into the resource field factory. I think this gets to your point about the logic not belonging to the repository while keeping the resource type a value object. WDYT?

P.S. There's also some cleanup of ResourceType to account for the new "smarter" fields. I think there's a lot of room for improvement/simplification in the code that we're talking about in 24.6 because of those same "smarts". I didn't make those yet in order to keep changes minimal, but we could make them in a subsequent interdiff if you're on board with this compromise.

Whoops, I forgot one commit in the last interdiff.

I still would like to see an explanation about the desire to keep ResourceType as a value object. I can't see a benefit that is really obvious to others.

As far as I recall it allowed for an easy persistent caching of the resource types as implemented here: #3018287: ResourceTypeRepository computes ResourceType value objects on *every request*. Still not committed though.

#28:

1. Can you expand on that with an example?
2. I agree that the names should be changed. I'm hesitant to use the word Definition because it reminds me too much of the fields and TypedData system. What about ResourceTypeAttribute and ResourceTypeRelationship?\

I still would like to see an explanation about the desire to keep ResourceType as a value object. I can't see a benefit that is really obvious to others.

It's not really about it being a value object. Even with the changes you made, ResourceType was still a value object. Once it was instantiated, its values didn't change and none of its getters would return different results based on external state.

My objection centers around how that pattern tightly couples construction of the ResourceType value object to the Entity and Field API and that it requires statically accessing any services we need. Those two things make our code difficult to adapt and extend.

Let's imagine that we want JSON:API Extras to be able to add synthetic relationship definitions and that those synthetic relationships could be created as a plugin. The pattern of deriving ResourceFields during ResourceType construction would mean that Extras's only option would be to extend and override ResourceType. Then, it'd have to add another static call like $relationship_plugin_manager = \Drupal::service('jsonapi_extras.sythetic_field.manager) somewhere, which would be called every time a new ResourceType was constructed. That's a little smelly on its own, but not terrible. The real difficulty comes from the fact that it would create a mutually exclusive class hierarchy. If another module wants to add support for an entity reference-like field (maybe DER or ERR) that JSON:API has trouble recognizing today, that module couldn't be installed in concert with JSON:API Extras.

In contrast, ResourceFieldFactory::createResourceFields() is trivial to decorate and permits any number of modules or systems to add their own fields without fussing about a class hierarchy:

public function createResourceFields() {
  $fields = $this->inner->createResourceFields();
  $new = new ResourceRelationship('field_unrecognized_custom_entity_reference');
  array_push($fields, $new));
  return $fields;
}

Do we need a change record for this issue?

This issue has been stagnant for too long! This functionality is a necessary component for JSON:API Schema. It's not for lack of enthusiasm that I haven't helped push this issue forward more quickly. After doing some reflection, I think the reason I haven't given it the attention it deserves is because I was a bit afraid of it. I had to take a moment and ask myself why.

I think it's because it's moving a lot of code around and introducing new concepts and internal APIs all at once. Lord knows I've been guilty of this myself in the past. I'm sorry for that.

So, how do we get this moving again and how do we accelerate that movement?

I think we need to break this down into more digestible pieces and commit them serially.

1. Introduce the definition objects that hold only the internal name, the public name and whether the field is enabled. IOW, let's replace the $field_mapping argument to ResourceType::__construct in a "pure" refactoring. There should be nothing new here.
2. Add the concept of cardinality (e.g. isMultiple) to the field objects.
3. Add the getResourceFields() and getResourceFieldByName($alias) methods.
4. Add the factory concept and interfaces.

I've written a patch for #1 with tests and comments. I think it's pretty much ready to go as-is. It'll require a followup in Extras because of the getFieldMapping => getFields change. I'll write that if and as soon as @e0ipso gives his blessing to this proposed plan. The same goes for creating the other issues.

No interdiff because it'd just be more confusing.

I'm marking this as a task because the patch is just a refactoring, not a new feature. Moving to the core queue for this patch.

1. +++ b/core/modules/jsonapi/src/ResourceType/ResourceType.php
   @@ -353,10 +344,12 @@ public function setRelatableResourceTypes(array $relatable_resource_types) {
   -    if (!isset($this->relatableResourceTypes)) {
   -      throw new \LogicException("setRelatableResourceTypes() must be called before getting relatable resource types.");
   -    }
   

   We probably don't want to lose this. Brought it back (in a different, more appropriate place now).

2. +++ b/core/modules/jsonapi/src/ResourceType/ResourceType.php
   @@ -371,10 +364,13 @@ public function getRelatableResourceTypes() {
   +    if (!isset($this->fieldMapping[$field_name])) {
   +      return [];
   +    }
   +    $internal_field_name = $this->fieldMapping[$field_name];
   +    return isset($this->fields[$internal_field_name]) && $this->fields[$internal_field_name] instanceof ResourceTypeRelationship
   

   Moved the if statement to a ternary cause it was bothering me.

3. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
   @@ -185,21 +185,14 @@ public function getByTypeName($type_name) {
   -   * @return array
   -   *   An array with:
   -   *   - keys are (real/internal) field names
   -   *   - values are either FALSE (indicating the field is not exposed despite
   -   *     not being internal), TRUE (indicating the field should be exposed under
   -   *     its internal name) or a string (indicating the field should not be
   -   *     exposed using its internal name, but the name specified in the string)
   +   * @return \Drupal\jsonapi\ResourceType\ResourceTypeField[]
   +   *   An array of JSON:API resource type fields keyed by internal field names.
   

   This change is the essence of this patch.

Ah, automated tests catch my sloppiness again lol. Fixed #38.

Wow, a lot happened since I last looked at this issue!

#37: this looks great! 👍 I wonder though if steps 2 and 3 should not also happen in this issue? I do think it makes sense to keep step 4 for a separate issue, because #37 demonstrates that this factory is not necessary. That means the reason for adding it must be that we want to add it to accommodate other modules, which would probably have to be a public API per #3032787: [META] Start creating the public PHP API of the JSON:API module.

I'm fine incorporating 2 & 3 if you think the extra code won't impede the patch. My thinking was that it'd be easier to land this as a clean refactoring. Then, for 2 & 3, we'd have the opportunity to refactor other bits of the codebase so we'd have the chance to actually use the new methods. Otherwise, we're just adding them without a usage and we'll have a refactor patch later anyway, unless we grow this patch bigger than #36 to also make use of the methods.

@e0ipso and @gabesullice discussed this in detail in person at DecoupledDays yesterday. They agreed about next steps. Let's document that agreement so that we don't forget about the details of it.

As I remember it, @e0ipso and I agreed that this lowered scope patch was okay if we were sure that we'd do the followups. The primary requirement of the followups is to enable JSON:API Schema to do its job, which is in fact my main reason for working on all of this. I'm committed to pushing forward and landing everything necessary for JSON:API Schema so that it can get a stable release.

Excellent. It may be helpful if you could post interdiffs for both steps 2 and 3. That would then allow a core committer to assess whether they want to include this in this patch like I suggested in #41 or not.

Here's the interdiff for step 3. Posting it here so tests can run while I work on step 2.

Whoa, thanks for the review @e0ipso! I'll address all that early Monday.

Here are files for steps 2 and 3.

In doing step 2, I realized it will depend on step 3. So we'll have to reverse the order of commit (unless they're all committed at once ofc!)

I made a change to the step 3 patch, I attached an interdiff for that too.

Before addressing #47 I'd like to get everything back to green.

3014277-48-with-get-fields-method.patch mostly fails because I forgot to add a use line to my commit. That line was added to the "with-both" patch, which is why it has so many fewer failures.

3014277-48-with-both.patch primary fails because of new required set-up in EntityResourceTest (this is pretty much the case for every meaningful change 😞)

The primary patch is unchanged.

On to addressing @e0ipso's review!

Okay, finally ready to post patches for @e0ipso's review. I found (and fixed) an existing bug that was revealed by 3014277-49-with-get-fields-method.patch. I fixed a piece of it in the step-2 interdiff, not realizing it was a more widely spread problem (i.e. CRUD on relationship routes does not correctly support field aliasing), which is why the with-both patch succeeds but the with-get-fields-method does not. I've updated interdiff-step-3-get-fields-method.txt to include that fix.

Now for @e0ipso's review...

1. That'd be nice! The only drawback is that you have to put a method name in as a string, which would break most static analysis tools and things like "go to method". It'd be really nice if PHP treated methods as values like JS does. Then you could do Util::curriedMethod(MyClass::method), passing the method as a "real" argument.

2. A man after my own heart. Done.

3. You posted this before I posted the interdiffs for steps 2 and 3. Normally I'd agree, but since those patches are already "done", I think adding this todo would be extra process + work.

4. 👍

5. I like this pattern because it keeps the object immutable. I also think it lends itself nicely to resource type repository decoration:

  protected function getFields(array $field_names, EntityTypeInterface $entity_type, $bundle) {
    return array_map(function (ResourceTypeField $field) {
      return ($alias = $this->getFieldAlias($field)) ? $field->withPublicName($alias) : $field;
    }, parent::getFields($field_names, $entity_type, $bundle));
  }

  protected function getFieldAlias(ResourceTypeField $field) {
    // return string if field is to be aliased, FALSE otherwise.
  }

6. Same as 5.

7. Again, for the immutable pattern.

8. I thought about this too but elected against it. I couldn't come up with a good reason that there should be ResourceTypeOmittedField but not ResourceTypeSingleField or ResourceTypeMultipleField other than because "null" ~= "omitted/disabled". Then I realized that the null pattern makes lots of sense for things that are meant to be values, but not things that define those values. IOW, OmittedResourceObject makes sense but OmittedResourceType does not. Also, since ResourceTypeFields are immutable, I think there's a little less need for the null pattern again.

9. Absolutely! Done.

Whoops, here's the interdiff specific to @e0ipso's review.

Thanks @e0ipso!

I updated the issue summary.

To recap for any further reviewers/committers...

There are lots of files in #50. The original patch was doing a lot. In #37 I wrote a new patch with a smaller scope because I thought it would make it easier and quicker to get it to RTBC. That patch has reached RTBC per #52 (3014277-50.patch). I proposed that after it landed, we could have 3 followups:

2. Introduce cardinality concept to ResourceTypeFields
3. Introduce methods to access ResourceTypeFields on a ResourceType
4. Add a decoratable ResourceTypeFieldFactory

@Wim Leers and @e0ipso thought that perhaps you (the committer reviewing this) would prefer to have steps 2 and 3 as part of this patch. So I began working on those as well. I discovered that step 2 actually depends on step 3. So the order of those will need to be reversed or they should be committed together. I haven't changed the original numbering so as not to create additional confusion.

Because we wanted to keep all options open, there are a number of files attached to #50. I'll briefly describe each one:

• interdiff-step-3-get-fields-method.txt: This would be the patch for the first followup that would introduce methods on the ResourceType class to make the fields accessible.
• interdiff-step-2-cardinality-method.txt: This would be the patch for the second followup that would introduce a concept of cardinality to the fields.
• 3014277-50-with-get-fields-method.patch: If you (the committer) feels the first followup could be committed together with this patch, this would be the combined patch.
• 3014277-50-with-both.patch: If you (the committer) feels the first & second followups could be committed together with this patch, this would be the combined patch (the second followup can't be committed without the first).
• 3014277-50.patch: This is the primary patch for this issue. It only covers the reduced scope that I proposed in #37 in order to make it easier to review and so that its followups would be smaller/easier to review as well.

I like the move to a more explicit API/data structure for this.
My only question is about the BC breaks.
Yes, I know the classes are internal, but for things like service constructors and plugin constructors we've been trying to ease the burden of minor updates by providing a BC layer, even though we're not obligated to. Do you think we should do the same here?

1. +++ b/core/modules/jsonapi/src/ResourceType/ResourceType.php
   @@ -82,28 +82,12 @@ class ResourceType {
   -  protected $disabledFields;
   ...
   -  protected $invertedFieldMapping;
   

   As we're removing these, should we provide a __get implementation that triggers a deprecation error if a sub-class tries to access these fields and then provides a fallback using the new API? That's the way we've typically removed properties in other sub-systems in a BC fashion

2. +++ b/core/modules/jsonapi/src/ResourceType/ResourceType.php
   @@ -160,8 +144,8 @@ public function getDeserializationTargetClass() {
   +    return isset($this->fields[$field_name])
   +      ? $this->fields[$field_name]->getPublicName()
          : $field_name;
   
   @@ -176,8 +160,8 @@ public function getPublicName($field_name) {
   +    return isset($this->fieldMapping[$field_name])
   +      ? $this->fieldMapping[$field_name]
          : $field_name;
   

   while we're touching these lines, I guess we could use the null coalesce operator now.

3. +++ b/core/modules/jsonapi/src/ResourceType/ResourceType.php
   @@ -318,17 +302,15 @@ public function __construct($entity_type_id, $bundle, $deserialization_target_cl
   +    $this->fields = $fields;
   ...
   -    $this->fields = array_keys($field_mapping);
   -    $this->disabledFields = array_keys(array_filter($field_mapping, function ($v) {
   -      return $v === FALSE;
   -    }));
   -    $this->fieldMapping = array_filter($field_mapping, 'is_string');
   -    $this->invertedFieldMapping = array_flip($this->fieldMapping);
   +    $this->fieldMapping = array_flip(array_map(function (ResourceTypeField $field) {
   +      return $field->getPublicName();
   +    }, $this->fields));
   

   should we be checking the type of the values in $fields argument in case a sub-class is passing the old value - we typically do this for constructor changes. If we find the wrong argument is being passed, we trigger_error and use the \Drupal singleton or similar to get the correct value

4. +++ b/core/modules/jsonapi/src/ResourceType/ResourceType.php
   @@ -353,10 +344,11 @@ public function setRelatableResourceTypes(array $relatable_resource_types) {
   +    return array_reduce(array_map(function (ResourceTypeRelationship $field) {
   +      return [$field->getPublicName() => $field->getRelatableResourceTypes()];
   +    }, array_filter($this->fields, function (ResourceTypeField $field) {
   +      return $field instanceof ResourceTypeRelationship;
   +    })), 'array_merge', []);
   

   is it worth statically caching/calculating this when ::setRelatableResourceTypes is called

5. +++ b/core/modules/jsonapi/src/ResourceType/ResourceType.php
   @@ -371,10 +363,10 @@ public function getRelatableResourceTypes() {
   +    $internal_field_name = isset($this->fieldMapping[$field_name]) ? $this->fieldMapping[$field_name] : FALSE;
   

   we could use null coalesce here too

6. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
   @@ -185,21 +185,14 @@ public function getByTypeName($type_name) {
   -  protected static function getFieldMapping(array $field_names, EntityTypeInterface $entity_type, $bundle) {
   

   should we be triggering an error here for sub-classes? the fact we have two sub-classes in core makes me think that custom/contrib code may also extend it (even though its internal code). should we try and prevent hard-breaks for those folks?

Thanks for the review @larowlan!

for things like service constructors and plugin constructors we've been trying to ease the burden of minor updates by providing a BC layer, even though we're not obligated to. Do you think we should do the same here?

JSON:API is littered with warnings about the fact that none of its PHP is to be considered BC-safe.

Given that:

1. JSON:API is so new to core
2. We're talking about a value object (ResourceType) and its factory (ResourceTypeRepository) as opposed to a plugin system (this design was specifically chosen to dissuade people from trying to alter them as they might alter plugins)
3. We have an established history of refactoring our internal APIs and public awareness about this policy
4. We have an issue where users can request pubic APIs for many of the features that decorating the ResourceTypeRepository would provide (#3032787: [META] Start creating the public PHP API of the JSON:API module)
5. There is already a module in contrib (JSON:API Extras) which lets users do everything they would be able to do by extending these classes. JSON:API Extras is maintained by @e0ipso and @Wim Leers and I regularly provide patches to it when we've changed the internal APIs that it relies upon. IOW, our established practice is to let Extras follow us, not to provide BC layers for it.

I truly don't believe the extra complexity these BC-layers would introduce are worth it.

1. See above.
2. 2. Done! I feel so... liberated! That's the first time I've been able to use null coalesce :D
3. See above.
4. I didn't cache it when setRelatableResourceTypes was called to avoid having the LogicException about calling setRelatableResourceTypes in two different places, but I did cache the first time it gets called.
5. ✅
6. See above.

I was about to post something similar as #3070204-22: Refactor the JSON:API FieldResolver to use a resource type instead of an entity type ID and bundle ID pair here but @Wim Leers's reply in #3070204-23: Refactor the JSON:API FieldResolver to use a resource type instead of an entity type ID and bundle ID pair pretty much covers my argument here as well so not having a BC should be fine for this issue.

For the BC question

OK I've looked over all three issues. In general I think the approach we've taken to adding bc layers to plugin constructors and similar has been good - while we don't have to, the bc layers are simple and easy to maintain in those cases and reduce disruption for contrib.

In this case, the code to maintain a bc layer would be considerably more complex, and the potential surface of people relying on the current behaviour is very small, so I do think it's OK to go ahead without one here. This doesn't mean there won't be other cases where we might want to provide bc though for @internal code if it's easy to do so.

Note I did not do an in-depth review of the patch yet so this shouldn't be taken as one.

Needs a reroll.

Rerolled.

Whoops, messed that up.

Gah, silly mistake. This really should be back to RTBC now.

Clarification: this blocks https://www.drupal.org/project/jsonapi_schema.

Bumping to major since this is a contrib blocker. Thanks!

Review credits

1. +++ b/core/modules/jsonapi/src/ResourceType/ResourceType.php
   @@ -389,4 +435,92 @@ public function getPath() {
   +    @trigger_error("Using the ${$name} protected property of a {$class_name} is deprecated in Drupal 8.8.0 and will not be allowed in Drupal 9.0.0. Use {$class_name}::getFields() instead. See https://www.drupal.org/project/drupal/issues/3014277.", E_USER_DEPRECATED);
   ...
   +    @trigger_error("Passing an array with strings or booleans as a field mapping to {$class_name}::__construct() is deprecated in Drupal 8.8.0 and will not be allowed in Drupal 9.0.0. See \Drupal\jsonapi\ResourceTypeRepository::getFields(). See https://www.drupal.org/project/drupal/issues/3014277.", E_USER_DEPRECATED);
   

   This needs to link to a change record - which we don't have yet - can you add that and update this error?

   Please put straight back to RTBC, I will keep an eye out for the issue.

2. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
   @@ -250,10 +264,47 @@ protected static function getFieldMapping(array $field_names, EntityTypeInterfac
   +    @trigger_error("{$class_name}::getFieldMapping() is deprecated in Drupal 8.8.0 and will not be allowed in Drupal 9.0.0. Use {$class_name}::getFields() instead. See https://www.drupal.org/project/drupal/issues/3014277.", E_USER_DEPRECATED);
   

   Same here too, thanks

CRs created:

1. https://www.drupal.org/node/3084721
2. https://www.drupal.org/node/3084746

Thanks @larowlan!

Doesn't apply after #3036285: Add a \JsonApiResource\Relationship object to carry relationship data, metadata and a link collection. - but also - the trigger_error in the code needs to use the CR links, not this issue link.

Needs work for both of those.

Ah, I didn't know about that practice. TIL.

Rerolled and made the trigger_error change per #73.

Committed 7dfe387 and pushed to 8.8.x. Thanks!

Fixed on commit

diff --git a/core/modules/jsonapi/src/Controller/EntityResource.php b/core/modules/jsonapi/src/Controller/EntityResource.php
index 67b5fb3f33..162ce068bf 100644
--- a/core/modules/jsonapi/src/Controller/EntityResource.php
+++ b/core/modules/jsonapi/src/Controller/EntityResource.php
@@ -48,7 +48,6 @@
 use Drupal\jsonapi\ResourceResponse;
 use Drupal\jsonapi\ResourceType\ResourceType;
 use Drupal\jsonapi\ResourceType\ResourceTypeField;
-use Drupal\jsonapi\ResourceType\ResourceTypeRelationship;
 use Drupal\jsonapi\ResourceType\ResourceTypeRepositoryInterface;
 use Drupal\jsonapi\Revisions\ResourceVersionRouteEnhancer;
 use Symfony\Component\HttpFoundation\Request;

Published both change records 😎

+++ b/core/modules/jsonapi/src/ResourceType/ResourceType.php
@@ -389,4 +435,92 @@ public function getPath() {
+   * @see https://www.drupal.org/project/drupal/issues/3014277

+++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
@@ -250,10 +264,47 @@ protected static function getFieldMapping(array $field_names, EntityTypeInterfac
+   * @see https://www.drupal.org/project/drupal/issues/3014277
...
+    @trigger_error("{$class_name}::getFieldMapping() is deprecated in Drupal 8.8.0 and will not be allowed in Drupal 9.0.0. Use {$class_name}::getFields() instead. See https://www.drupal.org/project/drupal/issues/3014277.", E_USER_DEPRECATED);

Should these all not point to https://www.drupal.org/node/3084746?

I think that indicates we're missing test coverage - should we roll this back?

I'd say let's create a followup as it is unblocking a bunch of issues.

+1 to what @jibran said. The error @e0ipso found is in the BC-layer on the internal ResourceType. I guess it's probably JSON:API Extras that's involved. I'll open a follow-up where we can find a good way to test it.

Done: #3085885: Follow-up to #3014277: Wrong variable used as argument in BC layer-providing ResourceType::updateDeprecatedFieldMapping()

Restoring status per @jibran & @gabesullice. See y'all in #3085885: Follow-up to #3014277: Wrong variable used as argument in BC layer-providing ResourceType::updateDeprecatedFieldMapping().

#77: @jibran, I don't think so. One is a deprecation on accessing internal properties, the other is a deprecation for constructing a resource type with an outdated argument.