🇺🇸United States @m.stenta

2. refactor simple_oauth to use access policies instead of RoleStorage::isPermissionInRoles()

It sounds like this might be underway in 🐛 6.0 is missing cache context + access policy support Active .

I opened a merge request that takes the approach described above. Setting this back to Needs Review.

Notably, I realized as I was doing this that it is still possible for the delete confirm form to display more than 10 entities, because it breaks them up by entity type, so rather than reworking all of that I just added a bit of logic to limit the number of entities it loads per type. So in theory it's still possible for this form to create an OOM error, but I think that would be an extreme edge case (multiple separate entity types referencing the same entity). We could take this further to ensure that only 10 entities are loaded overall (regardless of type), but that would require rethinking how we display the results. I decided not to go that far with this, since it should solve the primary issue for 99% of use-cases.

Since this only affects the FormAlter class now, and since that class doesn't have any test coverage to begin with, I'm going to remove the "Needs Tests" tag. It would be great to add tests for this confirmation form in general, but I don't think that needs to be a blocker for this. I tested it myself locally and it's working, but I'll leave it as "Needs Review" until someone else has a chance to test it themselves to confirm.

@andrew.wang: If you have a chance to test that would be great! Feel free to set to RTBC if it works for you and you agree with the approach.

Hmm one tricky bit: referentialEntityQuery() returns a list of "all entities of the specified type which have any fields in the source field list that match the given target ID". This gets called by getDependentEntityIds() once for each entity reference field, and then organizes them into sub-arrays by entity type.

So one call to getDependentEntityIds() results in potentially multiple calls to referentialEntityQuery(). But the $limit would be applied to each call to referentialEntityQuery()... which means calling it with a limit of 10 could result in more than 10 entities being returned.

This is actually true of the current patch as well.

So I think we have two options:

1. Add a $limit parameter only to getDependentEntities() (but not to getDependentEntityIds() or referentialEntityQuery()) and add logic to only load that many entities.
2. Don't add $limit parameters to any of the methods, and rework FormAlter::formAlter() so that it calls getDependentEntityIds() instead of getDependentEntities(), and load the first 10 ourselves for the purpose of displaying them in the confirmation form.

I think my preference would be option 2, because it doesn't change the API surface of any methods, and is a targeted fix for this issue.

It would still be good to add a test for this, to confirm that only 10 entities are shown in the form when there are more than 10 dependent entities. I don't think we necessarily need to expand the existing tests like I described in my previous comment, but it certainly wouldn't hurt either!

Thanks @andrew.wang! This makes sense to me. I'm going to re-categorize this as a bug report, because it is a potential OOM error.

Currently the patch changes EntityReferenceIntegrityEntityHandler::referentialEntityQuery() so that it always returns a maximum of 11 entities. This solves the OOM issue in the entity deletion form, as described above, but it could cause issues elsewhere. For example, if someone has downstream code that relies on this method to get a list of all entities, that would no longer work, and it wouldn't be clear why it's only returning 11.

The current automated test for this (in EntityReferenceIntegrityEntityHandlerTest::testGetDependentEntityIds()) would miss this because it only tests with a single dependent entity. We should probably expand those tests to look for multiple entities, to prevent issues like this from being introduced accidentally.

My assumption is the OOM error occurs when the entities are loaded (in EntityReferenceIntegrityEntityHandler::getDependentEntities()), not necessarily when the query itself is run, which only loads the IDs. If we can find a fix at that layer, and leave EntityReferenceIntegrityEntityHandler::referentialEntityQuery() as-is, then I think that would be ideal.

Tracing the logic to find the best place for this... the entity_reference_integrity_enforce module's FormAlter::formAlter() (which is responsible for adding the list of dependent entities to the delete confirmation form) calls $handler->getDependentEntities($entity); (on this line), before ultimately passing the result of that to FormAlter::buildReferencingEntitiesList(), which simply iterates over the (already loaded) list of entities and makes a list of links to them.

Side note: I like your approach of loading 11 entities, and then testing for >= 10 in that method. It's an elegant way to do it. So I think we can leave that part of your patch as-is, but we just need to rethink where we limit the query...

Perhaps when FormAlter::formAlter() calls EntityReferenceIntegrityEntityHandler::getDependentEntities(), it could specify a $limit argument in that method. This could then be passed through to EntityReferenceIntegrityEntityHandler::getDependentEntityIds(), and then to EntityReferenceIntegrityEntityHandler::referentialEntityQuery(). The $limit could default to 0 (no limit), and then we could set it in the context of FormAlter::formAlter(), so it only gets added where we need it.

Does that make sense?

The 2.x branch of this module supports Drupal 11. The 8.x-1.x branch will not.

The GitHub pull request has been approved and merged! 🎉

https://github.com/farmOS/farmOS/pull/967

And it includes the new D11-compatible release of JSON:API Schema! 🎉

I fixed all the level 2 errors.

Level 4 doesn't cause any errors, and level 5 only adds 5 more, so I'll try to fix those as well.

 ------ --------------------------------------------------------------------------------------------------------- 
  Line   web/modules/log/src/Form/LogActionFormBase.php                                                           
 ------ --------------------------------------------------------------------------------------------------------- 
  69     Parameter #1 $key of method Drupal\Core\TempStore\PrivateTempStore::get() expects string, int given.     
  124    Parameter #1 $key of method Drupal\Core\TempStore\PrivateTempStore::delete() expects string, int given.  
 ------ --------------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------------ 
  Line   web/modules/log/src/Plugin/Action/LogActionBase.php                                                   
 ------ ------------------------------------------------------------------------------------------------------ 
  70     Parameter #1 $key of method Drupal\Core\TempStore\PrivateTempStore::set() expects string, int given.  
 ------ ------------------------------------------------------------------------------------------------------ 

 ------ ------------------------------------------------------------------------------------------------------------------ 
  Line   web/modules/log/tests/src/Functional/LogActionsTest.php                                                           
 ------ ------------------------------------------------------------------------------------------------------------------ 
  304    Parameter #1 $callback of function array_map expects (callable(Drupal\Core\Entity\EntityInterface): mixed)|null,  
         Closure(Drupal\log\Entity\LogInterface): mixed given.                                                             
 ------ ------------------------------------------------------------------------------------------------------------------ 

 ------ -------------------------------------------------------------------------------------------------- 
  Line   web/modules/log/tests/src/Functional/LogCRUDTest.php                                              
 ------ -------------------------------------------------------------------------------------------------- 
  21     Parameter #1 $code of method Behat\Mink\WebAssert::statusCodeEquals() expects int, string given.  
 ------ -------------------------------------------------------------------------------------------------- 

                                                                                                                        
 [ERROR] Found 5 errors

I fixed all the level 2 errors.

Level 3 only adds 3 more errors, so let's fix those as well:

 ------ --------------------------------------------------------------------------------------------------------------------------------------- 
  Line   web/modules/log/src/Form/LogActionFormBase.php                                                                                         
 ------ --------------------------------------------------------------------------------------------------------------------------------------- 
  94     Method Drupal\log\Form\LogActionFormBase::getDescription() should return Drupal\Core\StringTranslation\TranslatableMarkup but returns  
         string.                                                                                                                                
 ------ --------------------------------------------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------------------------------------------ 
  Line   web/modules/log/tests/src/Functional/LogTestBase.php                                                                                
 ------ ------------------------------------------------------------------------------------------------------------------------------------ 
  32     PHPDoc type array of property Drupal\Tests\log\Functional\LogTestBase::$modules is not covariant with PHPDoc type array<string> of  
         overridden property Drupal\Tests\BrowserTestBase::$modules.                                                                         
         💡 You can fix 3rd party PHPDoc types with stub files:                                                                              
         💡 https://phpstan.org/user-guide/stub-files                                                                                        
                                                                                                                                             
  96     Method Drupal\Tests\log\Functional\LogTestBase::createLogEntity() should return Drupal\log\Entity\LogInterface but returns          
         Drupal\Core\Entity\EntityInterface.                                                                                                 
 ------ ------------------------------------------------------------------------------------------------------------------------------------

Level 2 errors:

 ------ --------------------------------------------------------------- 
  Line   src/Controller/LogAutocompleteController.php                   
 ------ --------------------------------------------------------------- 
  62     Call to an undefined method                                    
         Drupal\Core\Entity\EntityStorageInterface::getTableMapping().  
 ------ --------------------------------------------------------------- 
 ------ ---------------------------------------------------------- 
  Line   src/Entity/LogType.php                                    
 ------ ---------------------------------------------------------- 
  131    Call to an undefined method                               
         Drupal\Core\Entity\EntityStorageInterface::updateType().  
 ------ ---------------------------------------------------------- 
 ------ ------------------------------------------------------- 
  Line   src/Form/LogCloneActionForm.php                        
 ------ ------------------------------------------------------- 
  100    Variable $new_date in PHPDoc tag @var does not exist.  
 ------ ------------------------------------------------------- 
 ------ --------------------------------------------------- 
  Line   src/Form/LogForm.php                               
 ------ --------------------------------------------------- 
  111    Call to an undefined method                        
         Drupal\Core\Field\FieldItemInterface::getLabel().  
 ------ --------------------------------------------------- 
 ------ -------------------------------------------------------------- 
  Line   src/Form/LogRescheduleActionForm.php                          
 ------ -------------------------------------------------------------- 
  148    Call to an undefined method                                   
         Drupal\Core\Field\FieldItemInterface::isTransitionAllowed().  
  149    Call to an undefined method                                   
         Drupal\Core\Field\FieldItemInterface::applyTransitionById().  
  161    Call to an undefined method                                   
         Drupal\Core\Field\FieldItemInterface::isTransitionAllowed().  
  162    Call to an undefined method                                   
         Drupal\Core\Field\FieldItemInterface::applyTransitionById().  
 ------ -------------------------------------------------------------- 
 ------ ---------------------------------------------------------------- 
  Line   src/Form/LogTypeForm.php                                        
 ------ ---------------------------------------------------------------- 
  83     Call to an undefined method                                     
         Drupal\Core\Entity\EntityInterface::getDescription().           
  90     Call to an undefined method                                     
         Drupal\Core\Entity\EntityInterface::getNamePattern().           
  103    Call to an undefined method                                     
         Drupal\Core\Entity\EntityInterface::getWorkflowId().            
  110    Call to an undefined method                                     
         Drupal\Core\Entity\EntityInterface::shouldCreateNewRevision().  
 ------ ---------------------------------------------------------------- 
 ------ ----------------------------------------------------------- 
  Line   src/LogStorage.php                                         
 ------ ----------------------------------------------------------- 
  143    Call to an undefined method                                
         Drupal\Core\Entity\EntityInterface::getTypeNamePattern().  
 ------ ----------------------------------------------------------- 
 ------ ---------------------------------------------------------------- 
  Line   src/Plugin/views/field/LogField.php                             
 ------ ---------------------------------------------------------------- 
  30     Call to an undefined method                                     
         Drupal\views\Plugin\views\query\QueryPluginBase::addOrderBy().  
  31     Call to an undefined method                                     
         Drupal\views\Plugin\views\query\QueryPluginBase::addOrderBy().  
 ------ ---------------------------------------------------------------- 
 ------ ---------------------------------------------------------------- 
  Line   src/Plugin/views/sort/LogStandardSort.php                       
 ------ ---------------------------------------------------------------- 
  23     Call to an undefined method                                     
         Drupal\views\Plugin\views\query\QueryPluginBase::addOrderBy().  
  24     Call to an undefined method                                     
         Drupal\views\Plugin\views\query\QueryPluginBase::addOrderBy().  
  48     Call to an undefined method                                     
         Drupal\views\Plugin\views\query\QueryPluginBase::addOrderBy().  
  49     Call to an undefined method                                     
         Drupal\views\Plugin\views\query\QueryPluginBase::addOrderBy().  
 ------ ---------------------------------------------------------------- 
 ------ --------------------------------------------------------------------- 
  Line   tests/src/Functional/LogActionsTest.php                              
 ------ --------------------------------------------------------------------- 
  34     Call to method getQuery() on an unknown class                        
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  55     Call to method loadMultiple() on an unknown class                    
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  83     Call to method getQuery() on an unknown class                        
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  106    Call to method loadMultiple() on an unknown class                    
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  128    Call to method getQuery() on an unknown class                        
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  149    Call to method loadMultiple() on an unknown class                    
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  172    Call to method getQuery() on an unknown class                        
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  195    Call to method loadMultiple() on an unknown class                    
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  217    Call to method getQuery() on an unknown class                        
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  247    Call to method loadMultiple() on an unknown class                    
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  275    Call to method getQuery() on an unknown class                        
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  298    Call to method loadMultiple() on an unknown class                    
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
 ------ --------------------------------------------------------------------- 
 ------ --------------------------------------------------------------------- 
  Line   tests/src/Functional/LogCRUDTest.php                                 
 ------ --------------------------------------------------------------------- 
  46     Call to method getQuery() on an unknown class                        
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  52     Call to method load() on an unknown class                            
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  110    Call to method load() on an unknown class                            
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
 ------ --------------------------------------------------------------------- 
 ------ --------------------------------------------------------------------- 
  Line   tests/src/Functional/LogNamePatternTest.php                          
 ------ --------------------------------------------------------------------- 
  26     Call to method getQuery() on an unknown class                        
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  32     Call to method load() on an unknown class                            
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  52     Call to method getQuery() on an unknown class                        
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  58     Call to method load() on an unknown class                            
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
 ------ --------------------------------------------------------------------- 
 ------ --------------------------------------------------------------------- 
  Line   tests/src/Functional/LogTestBase.php                                 
 ------ --------------------------------------------------------------------- 
  25     Property Drupal\Tests\log\Functional\LogTestBase::$storage has       
         unknown class Drupal\group\Entity\Storage\GroupRoleStorageInterface  
         as its type.                                                         
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
  91     Call to method create() on an unknown class                          
         Drupal\group\Entity\Storage\GroupRoleStorageInterface.               
         💡 Learn more at https://phpstan.org/user-guide/discovering-symbols  
 ------ --------------------------------------------------------------------- 
 ------ ---------------------------------------------------------------------- 
  Line   tests/src/Kernel/LogActionsTest.php                                   
 ------ ---------------------------------------------------------------------- 
  77     Method Drupal\Core\Executable\ExecutableInterface::execute() invoked  
         with 1 parameter, 0 required.                                         
  80     Call to an undefined method                                           
         Drupal\Core\Entity\EntityInterface::get().                            
  94     Method Drupal\Core\Executable\ExecutableInterface::execute() invoked  
         with 1 parameter, 0 required.                                         
  97     Call to an undefined method                                           
         Drupal\Core\Entity\EntityInterface::get().                            
 ------ ---------------------------------------------------------------------- 
 [ERROR] Found 44 errors

🐛 Use Entity API query_access handler Needs review has been merged. I will tag a 3.0.0 release of Log shortly...

In the meantime, @paul121 has opened a PR that includes this issue's MR: https://github.com/farmOS/farmOS/pull/965

m.stenta → created an issue.

I am going to drop support for Drupal 9, and require Drupal 10.2+ for this.

m.stenta → created an issue.

But nevertheless it is a bug fix that changes current behavior, so I think it's worth a major version bump to make it clear to users of the Log module that they might want to check their implementations.

I have opened a 3.x branch for this.

I'm going to close this as "won't fix" in the Log module itself. We may pursue a UI solution for it in farmOS.

I'm going to close this as "won't fix" in the Log module itself. We may pursue a UI solution to this in farmOS itself. See related pull request: https://github.com/farmOS/farmOS/pull/637

This is related to but different from: ✨ Add link to cloned logs in status message Needs review

Cleaning up old issues. I think we can close this as "won't fix".

@paul121 opened a pull request for this: https://github.com/farmOS/farmOS/pull/965

public function addSavepoint($savepoint_name = 'mimic_implicit_commit')

Very naively (having just jumped into this issue for the first time myself), could we leverage the $savepoint_name method argument as a solution instead?

It sounds like the issue is primarily a conflict in the mimic_implicit_commit name itself. Presumably changing $savepoint_name = 'mimic_implicit_commit' to $savepoint_name = 'some_other_name' would also solve this problem? (I'm not suggesting that... just posing the thought.)

I don't know all of the code that calls addSavepoint(), but would it be possible to override that in the downstream modules that are affected by this issue, such that the default argument can be changes to avoid the conflict?

Just throwing out alternative ideas here to stimulate the conversation a bit, while I wrap my head around this issue myself. Apologies if these have already been explored. :-)

Can someone please update the issue summary to include an explanation of what the proposed change is to the logic, and why it is necessary? There are plenty of comments in this thread that say "the patch works!" but very little about what the broader implications of the change might be. Reading the code diff doesn't provide any of this, and it raises further questions.

Changing if ($this->inTransaction()) { to if ($this->inTransaction() && !$this->transactionManager()->has($savepoint_name)) means that the addSavepoint() logic will NOT run in certain scenarios anymore. Is this OK? Has thought been given to how this might behave in other contexts? If so, I don't see any of that documented here (unless I missed it?) and if this causes issues later, future developers will have a hard time looking back at this issue and understanding why the decision was made.

I'm not critiquing the solution itself - it might be the right answer! But I'd like to see more of an explanation so that it's easier to understand at a glance.

Setting back to "Needs work" for the issue summary update, per @smustgrave's comment #62 above.

8.x-1.0 has been released! 🎉

https://www.drupal.org/project/jsonapi_schema/releases/8.x-1.0 →

m.stenta → changed the visibility of the branch 8.x-1.x to hidden.

m.stenta → changed the visibility of the branch 3524246-fix-phpstan-errors to hidden.

Merged! Thanks everyone for helping with this!

We have automated tests now ( 📌 Add basic test coverage Active ), but neither PHP CodeSniffer nor PHPStan are flagging the t() usages.

In order to proceed with this I propose we:

1. Determine why PHP CodeSniffer / PHPStan are not flagging t().
2. Tweak testing config so that the usage of t() causes tests to fail.
3. Use the proposed changes to make the tests pass.

We have automated tests now ( 📌 Add basic test coverage Active ) so it would be good to include a test to demonstrate this issue and prevent regressions.

We have automated tests now ( 📌 Add basic test coverage Active ) so it would be good to include a test to demonstrate this issue and prevent regressions.

We have automated tests now ( 📌 Add basic test coverage Active ) so it would be good to include a test to demonstrate this issue and prevent regressions.

We have automated tests now ( 📌 Add basic test coverage Active ) so it would be good to include a test to demonstrate this issue and prevent regressions.

We have automated tests now ( 📌 Add basic test coverage Active ) so it would be good to include a test to demonstrate this issue and prevent regressions.

We have automated tests now ( 📌 Add basic test coverage Active ) so it would be good to include a test to demonstrate this issue and prevent regressions.

We have automated tests now ( 📌 Add basic test coverage Active ) so it would be good to include a test to demonstrate this issue and prevent regressions.

Updates:

The entity_reference_validators → module put out a new 2.0.0 → release that supports Drupal 11! Thanks @pcambra!

@e0ipso made me a maintainer of jsonapi_schema → , so we should be able to get a new release of that soon too!

Other than that, everything is ready to go! I opened a draft pull request on GitHub that is ready for review. It includes a TEMP ... commit that removes jsonapi_schema for testing purposes. Once there is a new release of that module, we can update it in this PR and remove that commit.

We have automated testing now ( 📌 Add basic test coverage Active ), so this will needs tests too.

Talked to @bradjones1 in Slack and we agreed to release 8.x-1.0 after 📌 Automated Drupal 11 compatibility fixes for jsonapi_schema Needs review is merged.

• 🐛 Ensure valid "type" values for JSON data types Active
• #3058851: Only add $ref to existing JSON Paths →
• 🐛 Config entities carry invalid object definitions Active
• ✨ Support for cross bundle resource types Needs work

These other issues listed in this roadmap can be fixed after that.

Specifically, replacing \Drupal:: calls with dependency injection requires changing the constructor arguments of DataDefinitionNormalizer, which would break any downstream classes that extend it. I would like to suggest we do that in 2.x and document it in the release notes as a breaking change. I may add an exception for that particular PHPStan rule so we can make everything else pass in the meantime.

Update: I've added a phpstan-baseline.neon file in 📌 Fix PHPStan errors Active that ignores the \Drupal calls in 8.x-1.x, so all PHPStan tests pass moving forward and we can avoid introducing any new errors. I opened a follow-up issue that removes phpstan-baseline.neon and replaces the \Drupal calls with dependency injection, which can be reviewed and considered for merging into 2.x as a breaking change: 📌 \Drupal calls should be avoided in classes, use dependency injection instead Active .

Opened a merge request, and tests all passed.

Marking this as "postponed" on 🌱 [Meta, Plan] Version 2.0 Active .

This is ready for merge, but must wait for 📌 Automated Drupal 11 compatibility fixes for jsonapi_schema Needs review .

m.stenta → created an issue.

Opened a second MR that adds a phpstan-baseline.neon file for ignoring the \Drupal calls. This allows us to acknowledge their existence, and instruct PHPStan to ignore them for the time being, while still flagging any future errors that new changes introduce.

I will open a follow-up issue with a MR for replacing the \Drupal calls with dependency injection, which can be considered for merge into 2.x as a breaking change.

Support for float and uri were added in 🐛 Decimal and float fields have a null schema Needs work and 🐛 Error: uri is not a valid type for a JSON document Active .

Updated title and setting to "Needs work" to refactor accordingly.

If we want to continue with this issue we will need to add automated tests for any new additions (now that 📌 Add basic test coverage Active is merged). A merge request would be preferable over patches.

Support also needed for "type": "metatag", "type": "uri" and "type": "float"

Support for float and uri were added in 🐛 Decimal and float fields have a null schema Needs work and 🐛 Error: uri is not a valid type for a JSON document Active .

I will follow up in the other issue you opened regarding other types: #3304201: Float, Metatag, URI and layout_section type fields have a empty schema →

I rebased this branch on top of latest 8.x-1.x (with merged 📌 Add basic test coverage Active ), and added a commit that changes DataDefinitionUndefinedNormalizer::extractPropertyData() so that it returns an array instead of an object. The declared return type is array, so it was wrong for it to return an object, and now that causes a fatal error with Symfony 7+.

Tests are all green (except PHPStan, which will be fixed separately: 📌 Fix PHPStan errors Active ). Setting this back to "Needs Review". Please review this most recent commit and set to RTBC so we can get this merged! :-)

I tried replicating this in the new automated tests, but wasn't able to get the code in the MR to trigger, so I think we would need to add more test coverage for this case.

Chatted briefly with @joachim in Slack:

@joachim: Sorry, it’s so long ago I don’t remember
@joachim: My issue came up with a custom data type, not a field type

I'm going to postpone this as "maintainer needs more info" for now. If anyone else runs into this and can provide a failing test to demonstrate it then we can reopen and work on adding support.

I think this is ready to merge. All tests are green, and all the context and reasoning is documented thoroughly above and in code comments.

I reached out to @joachim regarding #3332360: Add possible values for data types → to see if we can incorporate that too. If I don't hear back I may move forward with this regardless and we can tackle other cases later.

I've added tests for this. In doing so, I found that this patch was only working for base fields, not for config fields. It was a simple tweak to support both: instead of checking if the field storage definition is an instanceof ListDataDefinitionInterface, now it checks if the field definition is. Tests are included for both base and config field types now.

I think the easiest solution to this is to simply omit BooleanItem fields from being included in this logic, so that we do not add the oneOf property to them.

I also split the allowed values logic out (and streamlined/documented it) into an allowedValues() method on the DataDefinitionNormalizer class, and then subclassed that for boolean fields to disable allowed values. This removes the need to reference BooleanItem directly in DataDefinitionNormalizer, and it will allow field types that extend boolean to inherit the same default behavior, or override it if they want to.

Once we have support for Drupal 11 (see: 📌 Automated Drupal 11 compatibility fixes for jsonapi_schema Needs review ), we'll be able to run PHPStan tests via GitLab CI. I've already started working on fixing the existing issues. There aren't many, but some of them should be considered breaking changes. Specifically, replacing \Drupal:: calls with dependency injection requires changing the constructor arguments of DataDefinitionNormalizer, which would break any downstream classes that extend it. I would like to suggest we do that in 2.x and document it in the release notes as a breaking change. I may add an exception for that particular PHPStan rule so we can make everything else pass in the meantime.

Rebased this on top of latest 8.x-1.x, which now includes automated tests (via #3257911). Let's see if this breaks anything in the current tests, and then add new tests for this functionality specifically.

Confirmed this issue. It happens when there are greater than 2 allowed bundles in an entity reference field.

I created Page, Blog, and Article node types, and added an entity reference field to Articles called test_entity_reference that allows referencing any of them. The expected title on /api/node/article/resource/relationships/test_entity_reference/related/schema should be Article content item, Blog content item, and Page content item, but instead it is Page content item, and Page content item.

I will open a MR with a test for this and @symbioquine's fix.

Tests are green!

I added general tests for this module in 📌 Add basic test coverage Active , which allowed me to test this. I opened a MR that demonstrates the issue.

In testing this, I found that I had to remove the ResourceType type hint from both line 120 and 159. The automated tests caught both of them.

The GitLab CI test results show the same error:

There was 1 error:
1) Drupal\Tests\jsonapi_schema\Kernel\JsonApiSchemaTest::testJsonApiSchemaRelated
PHPUnit\Framework\Exception: Uncaught PHP Exception TypeError: "Drupal\jsonapi_schema\Controller\JsonApiSchemaController::Drupal\jsonapi_schema\Controller\{closure}(): Argument #1 ($type) must be of type Drupal\jsonapi\ResourceType\ResourceType, string given" at /builds/project/jsonapi_schema/src/Controller/JsonApiSchemaController.php line 159

https://git.drupalcode.org/project/jsonapi_schema/-/jobs/5260475

Now I will push a fix commit, so we can see tests pass.

Added a test for this.

Thanks @stefanlehmann - I'm going to fix this in 🐛 Decimal and float fields have a null schema Needs work , which was originally focused on float field types, but the fix is the same for decimal so I'll do both at once.

This also applies to decimal fields. Updating title and description accordingly.

Also, marking this as "Major" because including either of these field types in an entity bundle crashes the schema for that bundle with the following error. This may have changed recently due to upstream changes in Symfony, because I don't think I experienced this originally, but it was also reported in 🐛 Decimal field causes error message on custom entity Active .

The website encountered an unexpected error. Try again later.

TypeError: Symfony\Component\Serializer\Serializer::normalize(): Return value must be of type ArrayObject|array|string|int|float|bool|null, stdClass returned in Symfony\Component\Serializer\Serializer->normalize() (line 161 of /var/lib/tugboat/stm/vendor/symfony/serializer/Serializer.php).

Drupal\jsonapi_schema\Normalizer\ListDataDefinitionNormalizer->normalize(Object, 'schema_json', Array) (Line: 32)
Drupal\jsonapi_schema\Normalizer\FieldDefinitionNormalizer->normalize(Object, 'schema_json', Array) (Line: 161)
Symfony\Component\Serializer\Serializer->normalize(Object, 'schema_json', Array) (Line: 232)
Drupal\jsonapi_schema\Controller\JsonApiSchemaController->Drupal\jsonapi_schema\Controller\{closure}(Array, Object)
array_reduce(Array, Object, Array) (Line: 231)
Drupal\jsonapi_schema\Controller\JsonApiSchemaController->addFieldsSchema(Array, Object) (Line: 210)
Drupal\jsonapi_schema\Controller\JsonApiSchemaController->getResourceObjectSchema(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 638)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 53)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 116)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 90)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 741)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

I've learned a lot about how this module works since I posted the patch in #4. The correct approach is to provide a new normalizer class that declares supported types of decimal and float. I will open a merge request with tests shortly...

m.stenta → created an issue.

Thanks @jgaehring! I committed this along with updated tests.

Thanks @wimleers! Fixed.

m.stenta → created an issue.

I am noticing a similar issue in definitions/relationships/properties/roles/properties/data in the /jsonapi/user/user/resource/schema endpoint (which is not a config entity resource type, but relates to one):

Nevermind, this is supposed to be an array because it's describing a list of all the roles the user has.

definitions/attributes/properties/third_party_settings is missing "type": "object"

@symbioquine and @wimleers are correct: type is no longer missing, but it is not correct. It should be object instead of array.

I am noticing a similar issue in definitions/relationships/properties/roles/properties/data in the /jsonapi/user/user/resource/schema endpoint (which is not a config entity resource type, but relates to one):

Notice how it declares "type": "array" instead of object, and includes an items array. Just like in the third_party_settings example.

This does feel related to #3324824: Schema incorrect for config entity "fields" that are Maps and Sequences → as @wimleers pointed out ( https://www.drupal.org/project/jsonapi_schema/issues/3324824#comment-148... → ). I wonder if we should close this as "outdated" and work on fixing config entity normalization generally over there.

2. Some of the properties like: langcode, theme, region, … are incorrectly under definitions and they should be under definitions/attributes/properties/

This appears to be resolved in 8.x-1.x. Not sure when it was fixed, but they are all under definitions/attributes/properties/ now.

I forgot about the "related" schema endpoints (). I pushed some commits that add tests for them, as well as some general test reorganization.

I'm going to go ahead and merge this so that I can start using it to test/fix/merge other bugs/patches in the issue queue.

Test required relationships
Test other field types

These are done. Tests are green. I think we can consider this complete! Good enough for a start, anyway. :-)

This is where the stdClass object is coming from: https://git.drupalcode.org/project/jsonapi_schema/-/blob/ed083e4274feec8...

It appears that the (object) cast was originally added in this commit (which created the normalizers): 08a4b2f2 (from #3060299: Remove TypeMapper plugins in favor of normalizers → ).

... despite the fact that the parent method's docblock declares a return type of array: https://git.drupalcode.org/project/jsonapi_schema/-/blob/08a4b2f2b930a6a...

It appears @ tried to remove this in cd73c2f2, but then reverted that in d30041d3

So we have some things to detangle here... :-/

Setting this back to Needs Work because the automated tests I'm working on in 📌 Add basic test coverage Active revealed a bug that will affect Drupal 11 compatibility.

I have rebased on top of the commit in 📌 Add basic test coverage Active that causes the issue to demonstrate the failing test (notably it passes on 8.x-1.x).

There was 1 error:
1) Drupal\Tests\jsonapi_schema\Kernel\JsonApiSchemaTest::testJsonApiSchema
PHPUnit\Framework\Exception: Uncaught PHP Exception TypeError: "Drupal\jsonapi_schema\Normalizer\ComplexDataDefinitionNormalizer::normalize(): Return value must be of type ArrayObject|array|string|int|float|bool|null, stdClass returned" at /builds/project/jsonapi_schema/src/Normalizer/ComplexDataDefinitionNormalizer.php line 51
ERRORS!
Tests: 1, Assertions: 0, Errors: 1.

The declaration of normalize() return types revealed a bug with our current code. When the field module is enabled, there are cases where normalize() attempts to return a stdClass object, which is not allowed. I don't know the exact cause of this yet, but we'll have to figure it out before this can be merged.

This existing issue seems to be closely related: 🐛 Decimal field causes error message on custom entity Active

Installing the Field module - I started this but found that simply adding the Field module causes an unusual error (seems related to 🐛 Decimal field causes error message on custom entity Active ). This requires more digging to figure out what's happening.

Ah this turned out to be an issue because I was running tests with the changes necessary for Drupal 11 ( 📌 Automated Drupal 11 compatibility fixes for jsonapi_schema Needs review ). When I run against 8.x-1.x this works fine. This is something we will have to fix for D11, though, because that explicitly declares the return types of normalizer methods, and this module returns an invalid type (stdClass) in some cases. That's a separate issue...

For what it's worth, Drupal core does not consider constructor changes to be breaking: https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... →

However, we may still want to, for the sake of any downstream code that might be extending DataDefinitionNormalizer.

Opened a merge request with two commits (but this is built on top of 📌 Automated Drupal 11 compatibility fixes for jsonapi_schema Needs review , which in turn is built on [#], so all of those commits are included - they will need to merged before this).

The first commit simply adds drupal/jsonapi_hypermedia as a dev dependency of the module, because PHPStan couldn't find the classes that this module references.

The second commit replaces usage of \Drupal in classes with dependency injection. This change might be considered "breaking" because it changes the constructor arguments for all of the normalizers. Maybe there's another way to do this that is BC. If not, then perhaps we should wait and merge this ahead of a 2.0.0 release and document it as a breaking change (see 🌱 [Meta, Plan] Version 2.0 Active ).

Alternatively, we could remove this line entirely, which would avoid the need for all the changes: https://git.drupalcode.org/project/jsonapi_schema/-/blob/ed083e4274feec8...

It seems that phpstan isn't running automatically. I am going to punt that out to a separate issue.

Created a follow-up issue: 📌 Fix PHPStan errors Active

m.stenta → created an issue.

I discovered this too while writing automated tests in 📌 Add basic test coverage Active . I had to hard-code those same six entity types as exceptions in the tests to make them pass. If we merge this, we can remove that exception from the tests.

Adding these tests has already revealed one inconsistency that probably deserves it's own issue.

Oh I wonder if this is related: #3324769: Schema incorrect for config entity types with multi-part IDs (field_storage_config, field_config, entity_view_mode, entity_form_mode, entity_view_display and entity_form_display) →

Nevermind! 📌 Automated Drupal 11 compatibility fixes for jsonapi_schema Needs review will not require a breaking change after all. Disregard my last comment. :-)

It's important to understand what this means: if the current output is bad (eg: doesn't meet JSON Schema spec), then these tests are not going to catch those issues. They are primarily going to confirm that the module is working the way it does currently, good or bad.

Adding these tests has already revealed one inconsistency that probably deserves it's own issue. It seems that a lot of config entity types do not declare data.definitions.attributes.type (normally "object") or data.definitions.attributes.description (normally "Entity attributes") in their schema. For example, look at /jsonapi/entity_view_mode/entity_view_mode/resource/schema, and you'll see that data.definitions.attributes only has properties and additionalProperties sub-elements. But if you look at /jsonapi/user_role/user_role/resource/schema it also has type and description.

For now, I've added a condition to the tests to ignore the known problematic entity types, but ideally we can fix that and remove that condition in the future.

Tests are passing on both Drupal 10 and 11! 🎉

I think we still need to declare a minimum minor version of Drupal 10 (eg: ^10.1 or ^10.2).

As far as I can tell, the only change that requires this is the one related to https://www.drupal.org/node/3359695 → , which was introduced in Drupal 10.1. I pushed an amended commit that updates the version constraint to ^10.1.

This is ready for final review! (But of course must wait for 📌 Add basic test coverage Active to be merged.) If anyone has other concerns/considerations, now is the time to voice them! If I don't hear anything by next week then I will assume the previous RTBC applies, along with the newly restored Drupal 10 support per @dmitry.korhov's feedback.

Thanks all!