Drupal 9 Deprecated Code Report

Looks good

Hi!
With using drupal-check I still see such errors:

------ ----------------------------------------------------------------- 
  Line   src/Plugin/jsonapi/FieldEnhancer/DateTimeFromStringEnhancer.php  
 ------ ----------------------------------------------------------------- 
  31     Call to deprecated function drupal_get_user_timezone():          
         in drupal:8.8.0 and is removed from drupal:9.0.0. Use            
         date_default_timezone_get() instead.                             
 ------ ----------------------------------------------------------------- 
 ------ --------------------------------------------------------- 
  Line   src/ResourceType/ConfigurableResourceTypeRepository.php  
 ------ --------------------------------------------------------- 
  272    Call to deprecated method getFieldMapping() of class     
         Drupal\jsonapi\ResourceType\ResourceTypeRepository:      
         in drupal:8.8.0 and is removed from drupal:9.0.0. Use    
         self::getFields() instead.                               
 ------ --------------------------------------------------------- 
 ------ ------------------------------------------------------------------- 
  Line   tests/src/Kernel/Controller/EntityResourceTest.php                 
 ------ ------------------------------------------------------------------- 
  185    Call to deprecated method setExpectedException() of class          
         Drupal\KernelTests\KernelTestBase:                                 
         in drupal:8.8.0 and is removed from drupal:9.0.0.                  
         Backward compatibility for PHPUnit 4 will no longer be supported.  
 ------ ------------------------------------------------------------------- 
 ------ ----------------------------------------------------------------------------------- 
  Line   tests/src/Kernel/EntityToJsonApiTest.php                                           
 ------ ----------------------------------------------------------------------------------- 
  214    Call to deprecated method assertInternalType() of class PHPUnit\Framework\Assert:  
         https://github.com/sebastianbergmann/phpunit/issues/3369                           
  219    Call to deprecated method assertInternalType() of class PHPUnit\Framework\Assert:  
         https://github.com/sebastianbergmann/phpunit/issues/3369                           
  238    Call to deprecated method assertInternalType() of class PHPUnit\Framework\Assert:  
         https://github.com/sebastianbergmann/phpunit/issues/3369                           
 ------ ----------------------------------------------------------------------------------- 

please review

Patch applies cleanly

martin@martin-XPS-13-9370 /var/www/html/brown.localhost/web/modules/contrib/jsonapi_extras (8.x-3.x=) $ git apply -v jsonapi_extras-3069220-6.patch
Checking patch src/Form/JsonapiResourceConfigDeleteForm.php...
Checking patch src/Form/JsonapiResourceConfigForm.php...
Checking patch src/Plugin/jsonapi/FieldEnhancer/DateTimeFromStringEnhancer.php...
Checking patch src/ResourceType/ConfigurableResourceTypeRepository.php...
Checking patch tests/src/Kernel/Controller/EntityResourceTest.php...
Checking patch tests/src/Kernel/EntityToJsonApiTest.php...
Applied patch src/Form/JsonapiResourceConfigDeleteForm.php cleanly.
Applied patch src/Form/JsonapiResourceConfigForm.php cleanly.
Applied patch src/Plugin/jsonapi/FieldEnhancer/DateTimeFromStringEnhancer.php cleanly.
Applied patch src/ResourceType/ConfigurableResourceTypeRepository.php cleanly.
Applied patch tests/src/Kernel/Controller/EntityResourceTest.php cleanly.
Applied patch tests/src/Kernel/EntityToJsonApiTest.php cleanly.

Still, 10 errors to solve

martin@martin-XPS-13-9370 /var/www/html/brown.localhost/web/modules/contrib $ drupal-check jsonapi_extras/
 47/47 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------- 
  Line   src-impostor-normalizers/ConfigEntityDenormalizerImpostor.php                                                                                           
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------- 
         Class Drupal\jsonapi\Normalizer\ImpostorFrom\jsonapi_extras\ConfigEntityDenormalizerImpostor was not found while trying to analyse it - autoloading is  
         probably not configured properly.                                                                                                                       
         💡 Learn more at https://phpstan.org/user-guide/autoloading                                                                                              
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------------------------------------------------------------------------------------------- 
  Line   src-impostor-normalizers/ContentEntityDenormalizerImpostor.php                                                                                           
 ------ --------------------------------------------------------------------------------------------------------------------------------------------------------- 
         Class Drupal\jsonapi\Normalizer\ImpostorFrom\jsonapi_extras\ContentEntityDenormalizerImpostor was not found while trying to analyse it - autoloading is  
         probably not configured properly.                                                                                                                        
         💡 Learn more at https://phpstan.org/user-guide/autoloading                                                                                               
 ------ --------------------------------------------------------------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------ 
  Line   src-impostor-normalizers/FieldItemNormalizerImpostor.php                                                                                                    
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------ 
         Class Drupal\jsonapi\Normalizer\ImpostorFrom\jsonapi_extras\FieldItemNormalizerImpostor was not found while trying to analyse it - autoloading is probably  
         not configured properly.                                                                                                                                    
         💡 Learn more at https://phpstan.org/user-guide/autoloading                                                                                                  
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------ 

 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------ 
  Line   src-impostor-normalizers/ResourceIdentifierNormalizerImpostor.php                                                                                           
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------ 
         Class Drupal\jsonapi\Normalizer\ImpostorFrom\jsonapi_extras\ResourceIdentifierNormalizerImpostor was not found while trying to analyse it - autoloading is  
         probably not configured properly.                                                                                                                           
         💡 Learn more at https://phpstan.org/user-guide/autoloading                                                                                                  
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------------ 

 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------- 
  Line   src-impostor-normalizers/ResourceObjectNormalizerImpostor.php                                                                                           
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------- 
         Class Drupal\jsonapi\Normalizer\ImpostorFrom\jsonapi_extras\ResourceObjectNormalizerImpostor was not found while trying to analyse it - autoloading is  
         probably not configured properly.                                                                                                                       
         💡 Learn more at https://phpstan.org/user-guide/autoloading                                                                                              
 ------ -------------------------------------------------------------------------------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------------------------------------------- 
  Line   src/Normalizer/SchemaFieldDefinitionNormalizer.php                                                                     
 ------ ----------------------------------------------------------------------------------------------------------------------- 
         Class Drupal\schemata_json_schema\Normalizer\jsonapi\FieldDefinitionNormalizer not found and could not be autoloaded.  
         💡 Learn more at https://phpstan.org/user-guide/autoloading                                                             
  14     Class Drupal\schemata_json_schema\Normalizer\jsonapi\FieldDefinitionNormalizer not found and could not be autoloaded.  
         💡 Learn more at https://phpstan.org/user-guide/autoloading                                                             
 ------ ----------------------------------------------------------------------------------------------------------------------- 

 ------ ---------------------------------------------------------------------------------------------------------------------- 
  Line   src/Normalizer/SchemataSchemaNormalizer.php                                                                           
 ------ ---------------------------------------------------------------------------------------------------------------------- 
         Class Drupal\schemata_json_schema\Normalizer\jsonapi\SchemataSchemaNormalizer not found and could not be autoloaded.  
         💡 Learn more at https://phpstan.org/user-guide/autoloading                                                            
  12     Class Drupal\schemata_json_schema\Normalizer\jsonapi\SchemataSchemaNormalizer not found and could not be autoloaded.  
         💡 Learn more at https://phpstan.org/user-guide/autoloading                                                            
 ------ ---------------------------------------------------------------------------------------------------------------------- 

 ------ ----------------------------------------------------------------------------------- 
  Line   tests/src/Functional/JsonApiExtrasFunctionalTest.php                               
 ------ ----------------------------------------------------------------------------------- 
  161    Call to deprecated method assertInternalType() of class PHPUnit\Framework\Assert:  
         https://github.com/sebastianbergmann/phpunit/issues/3369                           
 ------ ----------------------------------------------------------------------------------- 

                                                                                                                        
 [ERROR] Found 10 errors

9 errors are identical, so I guess effectively it are just two issues to solve.

After running composer update within the module folder, letting Composer generate an autoload.php file in the local vendor folder, it gives additional errors:

------ ------------------------------------------------------------------------------------------------------------------------------------------- 
  Line   vendor/e0ipso/shaper/tests/src/Validator/ValidateableBaseTest.php                                                                          
 ------ ------------------------------------------------------------------------------------------------------------------------------------------- 
         Class Shaper\Tests\Validator\FakeValidateable was not found while trying to analyse it - autoloading is probably not configured properly.  
         💡 Learn more at https://phpstan.org/user-guide/autoloading                                                                                 
 ------ -------------------------------------------------------------------------------------------------------------------------------------------

My knowledge of the Composer's autoload mechanism is too limited right now to come up with a solution, but the info might trigger others to continue optimizing autoload functionality for this module in some way. Any info would be appreciated. I keep following the issue with interest.

Added some updates to patch supplied in comment #6

Patch in #6 & #9 breaks my site specifically when i try to clear caches. I get the following error.

 [warning] assert(): assert(is_bool($public_field_name) || is_string($public_field_name)) failed ResourceType.php:512
 [warning] assert(): assert(is_bool($public_field_name) || is_string($public_field_name)) failed ResourceType.php:512
 [warning] assert(): assert(is_bool($public_field_name) || is_string($public_field_name)) failed ResourceType.php:512

Fatal error: Uncaught TypeError: Argument 1 passed to Drupal\jsonapi\ResourceType\ResourceType::Drupal\jsonapi\ResourceType\{closure}() must be an instance of Drupal\jsonapi\ResourceType\ResourceTypeField, bool given in /app/docroot/core/modules/jsonapi/src/ResourceType/ResourceType.php:363
Stack trace:
#0 [internal function]: Drupal\jsonapi\ResourceType\ResourceType->Drupal\jsonapi\ResourceType\{closure}(false)
#1 /app/docroot/core/modules/jsonapi/src/ResourceType/ResourceType.php(365): array_map(Object(Closure), Array)
#2 /app/docroot/modules/contrib/jsonapi_extras/src/ResourceType/ConfigurableResourceTypeRepository.php(150): Drupal\jsonapi\ResourceType\ResourceType->__construct('file', 'file', 'Drupal\\file\\Ent...', false, true, true, false, Array)
#3 /app/docroot/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php(124): Drupal\jsonapi_extras\ResourceType\ConfigurableResourceTypeRepository->createResourceType(Object(Drupal\Core\Entity\ContentEntityType), 'file')
#4 /app/docroot/core/modules/jsonapi/src/Ro in /app/docroot/core/modules/jsonapi/src/ResourceType/ResourceType.php on line 363

Would be good to know how to fix this. I think it's because self::getFields has been used in the overrideFieldMapping method in

jsonapi_extras/src/ResourceType/ConfigurableResourceTypeRepository.php 

. The array returned now contains a mixture of field names (as keys) with booleans or strings as their values and ResourceTypeField items. Would be good to have some direction on how to fix this :) as i'm happy to help out.

Scan with https://github.com/palantirnet/drupal-rector found a few issues to fix. Patch is attached.

added fixes for issues from drupal-check

With #14 applied drupal-check, rector and upgrade-status report no deprecation issue.

Also, I was able to enable the module on 8.8.6 and drupal 9.0.0-rc1 and clear cache on both versions.

As there were several concerns above, waiting for more checks and approves.

Tested patch #13 on vanilla Drupal versions 8.8.6 and 9.0.x-dev. I get the same error as described in #10.

I installed the module and disabled the body field for the page node via `admin/config/services/jsonapi/add/resource_types/node/page`

I think the problem is the usage of using the getFields method inside overrideFieldMapping . It feels like there is more to change to get this working. I am not sure the best way to do this. Happy to take another look if I get some direction on this.

Thanks

I also reproduced the issue reported at #10. It happens when some of the fields are disabled in the Resource Override form.

I tried to exclude these fields in ConfigurableResourceTypeRepository::overrideFieldMapping method but it caused errors later in ResourceType::setRelatableResourceTypes.

As an ad hoc solution, I copied the logic of deprecated ResourceTypeRepository::getFieldMapping function in ConfigurableResourceTypeRepository::overrideFieldMapping and it helped.
But I'm sure that this issue needs a deeper investigation.

I've started to look into this, not entirely sure yet, but i want to put down my current thoughts so i won't forget.

It seems Drupal\jsonapi\ResourceType\ResourceType has changes 10-2019 where the logic to populate the fieldMappings has changed. Before it did a check to clean up the array of anything that was false.

    $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);

That changed to an array map with a typehinted closure in issue https://www.drupal.org/project/drupal/issues/3014277 . The new code:

    $this->fieldMapping = array_flip(array_map(function (ResourceTypeField $field) {
      return $field->getPublicName();
    }, $this->fields));

So it seems the module is needs to adjusted to reflect this change. Haven't looked into what changed exactly and how this should be fixed, but I feel the solution is close.

Ok, so i changed how the override works so it uses the correct API from Core JSON:API. This should at least alleviate the issues as reported in #10. I'll get back to this later.

In order to get this working on newer versions of Drupal (and 9 also) i needed to implement some changes from another issue to get things up and running. (issue https://www.drupal.org/project/drupal/issues/2996114). I've pulled some code from the ResourceRepository to extra's to see if this resolves the issues. There was one test failing locally which was kinda weird. But first lets see how it does.

When testing it seems there might be a caching issues left. When changing a field enhancer (and only a field enhancer) the output of the api is not updated.

Fixed a test that was using the backwards compatibility layer on 8.9.x

Had a look at the caching issue with my colleage @casey and we found a way to add a caching tag when a custom enhancer is added. Yay! Think all issues should be fixed now.

Looking good! Two minor things i noticed:

1. +++ b/src/Plugin/jsonapi/FieldEnhancer/DateTimeFromStringEnhancer.php
   @@ -28,7 +28,7 @@ class DateTimeFromStringEnhancer extends DateTimeEnhancerBase {
   +      $output_timezone = new \DateTimezone(date_default_timezone_get());
   

   This looks like it's breaking backwards compatibility

2. +++ b/src/ResourceType/ConfigurableResourceTypeRepository.php
   @@ -266,12 +262,118 @@ class ConfigurableResourceTypeRepository extends ResourceTypeRepository {
   +          \sprintf(
   

   I would not use the full FQN because no where else is this used for functions in the global namepsace

1. drupal_get_user_timezone() is deprecated in drupal:8.8.0. It will be removed from drupal:9.0.0. Use date_default_timezone_get() instead. See https://www.drupal.org/node/3009387

   Seems it should be the function in 8.9 even just returns the result from the native function.

2. Good point, will change the function call.

Changed the \sprintf call.

As a sidenote, because this patch also applies changes to the .info.yml it will not apply through composer patches as-is. If you need this before it has been released, copy the patch locally and remove the section dealing with .info.yml. Take note there are two .info.ymls in the project.

As for a review, I know little about the inner workings of JSON:API and JSON:API Extras, so I can only comment on the findings of code analysis tools, what the code looks like and whether I actually have a fucntioning project :)

As for the code, I have one small nit to pick:

+  /**
+   * Get relatable resource types from a field definition.
+   *
+   * @inheritDoc
+   */

For consistency, this should probably just be an {@inheritdoc} like the function above it. Like that other method, this is overriding a method from the base class which has full documentation there.

Drupal-check complains about some of the impostor classes not being loaded, which makes sense since that mechanism is sort of hacky. No actual usages of deprecated code left.

And last but not least, I now get some nice errors about resources not existing (yet) instead of a hard exception (see Comment #143 of Argument 2 passed to Drupal\jsonapi\Routing\Routes::Drupal\jsonapi\Routing\{closure}() must be an instance of Drupal\jsonapi\ResourceType\ResourceType, NULL given) for the issue that brought me on this patch's path.

Apart from the small nit above I think this is good to go.

Maybe either @tresti88 or @kolesnikoff want to have a look at whether their issue is resolved as well.

Thanks for having a look Eelke, i've removed the comment from the docblock so it's consistent. I'm not too sure how I can get the exception better really. The current core implementation to getting the resource also just returns NULL when it can't find it.

I'll wait for potential extra reviews until later this week otherwise i'll get this merged to help the linked issue and get the issue queue on extra's going again.

Sorry to be a pain, but it is still not consistent with the previous method, plus it doesn't actually match the coding guidelines: https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

It should be

  /**
   * {@inheritdoc}
   */

But it currently is

  /**
    * @inheritDoc
    */

Note the use of braces and upper case. Sorry, that's far too many lines of comment for such a small thing 🙈

For the record, I didn't mean the comment about the errors as a negative thing, it makes perfect sense like this. As long as the errors disappear once the config import completes I am totally happy 👍

Oops, guess I should patch 5 minutes before a meeting hehe, sorry, thanks. :)

Not quite there yet...

  /**
   * Gets all bundle IDs for a given entity type.
   *
   * {@inheritdoc}
   */

should be:

  /**
   * {@inheritdoc}
   */

No comment... o_O

Same :)

Thanks everyone for the help, Mergin into dev :)

Adding @casey for credit since he helped find the caching issue.
Adding @BR0kEN since part of my patch was based on his work in https://www.drupal.org/project/drupal/issues/2996114

Bad news. It looks like the 3.15 release does not solve the issue that #26 did solve for us... Something must have changed significantly since then.

The changes in 3.15 after #26 consist of only some changes in composer.json (author, license) and the docbocks. There have been no other changes. When I try to apply the patch tot the current codebase it also results in no changed files...

Strange. I will have another look on monday. No time today, unfortunately.

I had a litlte harder look. Believe it or not, but the removal of the backslash (since #26) in front of sprintf is the culprit. It may be down to the PHP version, we're running on PHP 7.2.

First I examined the difference between 3.15 and 3.14 patched with #26. This seemed the only change that was remotely likely to have anything to do with the error returning. I then redid the test with 3.15 (still the error, like this morning, so I didn't imagine it) and then added the backslash back in front of the sprintf() in 3.15. And there were the error messages again (i.e. the desired behaviour) of resources missing and the config import completing. 🤯

Wow... i will release a fix tonight. And try to understand why....

I wonder where the backslash came from in the first place and why it was added.

It was part of the code copied from #2996114: Argument 2 passed to Drupal\jsonapi\Routing\Routes::Drupal\jsonapi\Routing\{closure}() must be an instance of Drupal\jsonapi\ResourceType\ResourceType, NULL given, where it was introduced in #123. Has been part of the patch over there since it was introduced.

My assumption is that there is a sprinf in the scope there, although i would not for the life of me know why. It also kinda weird me out that this is the only instance where the slash is added.

The backslash is added by me to the patch I posted for Drupal core is accidental occasion, however I use them for built-in stuff in custom code all the time since their presence explicitly says “take that function/constant/etc from the PHP globals”.

As you may notice, Drupal, - neither core nor contrib - doesn’t use that pattern. The only case I could imagine as of now is that somewhere in your code exists a “sprintf” that overrides the built-in. Try to check whether the “\spintf” and “spintf” in that namespace point to the exact same function.

Another thing I can think of is OPCache. Ensure you didn’t hit it in between the tests.

To sum up: I don’t think the backslash should be added to the jsonapi_extras and we definitely should find the real root cause.

Yeah I think you are right BR0kEN, I can't reproduce and cannot find any deifnition of a function or method in the code as is. This has to be something in your scope @eelkeblok

Cleaning up since i cannot reproduce and it really seems like it is something local to you @eelkeblok

Hmm. Strange. Still, good to know more about the source of that backslash. Note that the error did not actually involve the backslash itself, or the sprintf function. It was just the same error as reported in #2996114: Argument 2 passed to Drupal\jsonapi\Routing\Routes::Drupal\jsonapi\Routing\{closure}() must be an instance of Drupal\jsonapi\ResourceType\ResourceType, NULL given. The OPCache might be the best guess and it was just a matter of the system getting confused about what to call because the backslash went away.