Make BaseFieldOverride inherit internal property from the base field

Patch works great. Cleared cache right after running patch and path showed right up for me on jsonapi routes.

Failed tests are unrelated.

I'm experiencing this problem, and since the JSON:API is so critical in providing content for my GatsbyJS frontend it's really hurting my attempts at generating a multi-lingual site.

Patch worked for me, thank you!

I've run the test against 9.5.x-dev and seems the error is unrelated.

RTBC for the #2 patch.

Back to RTBC as per comments #7, #9.

This makes sense to me but it'd be great to add some test coverage. I would have thought it would be possible to add coverage of this to \Drupal\KernelTests\Core\Field\Entity\BaseFieldOverrideTest

The internal property is also used by the default content module to determine what field to export.

I made some changes to the provided patch as per this issue https://www.drupal.org/project/drupal/issues/2321071
Also I would suggest to change the syntax for the 10.x branch to return $base_field_definition?->isInternal(); since we have php8+ goodies there.
For that reason I created a second branch for the 10.1.x

No pr yet, since tests are pending.

I ended up making a new branch for 9.5.x since the rebase was taking forever :S
I made the proposed changes. Let me know if it's good to go, so that I can apply the same changes to the 10.x branch
Cheers!

applied patch #2 in Drupal 10.1.x

applied patch #2 in Drupal 10.1.x

@shubham-chandra The changes are in a pr for the 10.1.x branch already, but in case you need a patch, there you go.

Seems like the js failing tests issue is fixed (?)
I will be changing this to needs review

Reviewed and tested, screenshot attached as evidence.

The implementation of isInternal() should be return $this->getBaseFieldDefinition()->isInternal();. This method should never return NULL and getBaseFieldDefinition is guaranteed to either return a definition or error because an array key is not set. The change should be the same for 9.5.x and 10.x

I will change it back then. I included the null-safe because I saw that a similar approach was used here

I will add a note to this issue as well.

That issue is changing how getBaseFieldDefinition() works - if that one lands after this it'll need to be make the change but and vice versa but that issue has not landed yet.

I made the suggested corrections.

We could also add return type hinting for bool at the 10.x branch, or is it better to do it later in a different issue for more functions than just this (If at all) ?

Let's not add return typehinting... for new methods / interfaces sure but exsiting stuff let's leave alone.

Can you post a patch some we can test on multiple branches? Thanks.

Sure!

9.5 has some js test failure (AFAI can understand) and the second run passed.
Is there anything else to be done?

public function isInternal() {
Should have a typehint return

Also adding a new function will require a change record.

Added type hind in isInternal method, keeping the status needs work for change record.

I unpublished the change record.

Setting to Needs review.

Fixed up the Change record some.

Rest looks fine.

@efpapado that MR against 11.x is not necessary. The MR against 10.1.x cna be applied to 11.x just fine.

Committed and pushed f91e76e505e to 11.x and 4e682c3a68e to 10.2.x. Thanks!

Discussed with @longwave - we agreed to backport to 10.1.x and remove the CR as it is not that helpful in this case.

This, for example, will remove the field from JSON:API results and might have other undesired consequences.

😮 wow, this was an important oversight in #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts! Nice find.