Problem/Motivation

The module use the reserved word "Resource" as part of namespace, and it's generate a warning compatibility with php 7+.
See: http://php.net/manual/en/reserved.other-reserved-words.php

FILE: /jsonapi_resources/src/Resource/EntityQueryResourceBase.php
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 3 | WARNING | 'resource' is a soft reserved keyword as of PHP version 7.0 and should not be used to name a class, interface or trait or as part of a namespace (T_NAMESPACE)
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /jsonapi_resources/src/Resource/EntityResourceBase.php
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 3 | WARNING | 'resource' is a soft reserved keyword as of PHP version 7.0 and should not be used to name a class, interface or trait or as part of a namespace (T_NAMESPACE)
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /jsonapi_resources/src/Resource/ResourceBase.php
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 3 | WARNING | 'resource' is a soft reserved keyword as of PHP version 7.0 and should not be used to name a class, interface or trait or as part of a namespace (T_NAMESPACE)
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /jsonapi_resources/tests/modules/jsonapi_resources_test/src/Resource/AddComment.php
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 3 | WARNING | 'resource' is a soft reserved keyword as of PHP version 7.0 and should not be used to name a class, interface or trait or as part of a namespace (T_NAMESPACE)
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /jsonapi_resources/tests/modules/jsonapi_resources_test/src/Resource/AuthorArticles.php
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 3 | WARNING | 'resource' is a soft reserved keyword as of PHP version 7.0 and should not be used to name a class, interface or trait or as part of a namespace (T_NAMESPACE)
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /jsonapi_resources/tests/modules/jsonapi_resources_test/src/Resource/FeaturedNodes.php
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 3 | WARNING | 'resource' is a soft reserved keyword as of PHP version 7.0 and should not be used to name a class, interface or trait or as part of a namespace (T_NAMESPACE)
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /jsonapi_resources/tests/modules/jsonapi_resources_test/src/Resource/AddReminder.php
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 3 | WARNING | 'resource' is a soft reserved keyword as of PHP version 7.0 and should not be used to name a class, interface or trait or as part of a namespace (T_NAMESPACE)
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /jsonapi_resources/tests/modules/jsonapi_resources_test/src/Resource/CurrentUserInfo.php
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 3 | WARNING | 'resource' is a soft reserved keyword as of PHP version 7.0 and should not be used to name a class, interface or trait or as part of a namespace (T_NAMESPACE)
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Steps to reproduce

run ./vendor/bin/phpcs -p jsonapi_resources/ --standard=PHPCompatibility --extensions=php,module,inc,install,test,profile,theme

Proposed resolution

Change the name from "Resource" to "JsonApiResource", the same happened with the json api core module, you can check here: https://www.drupal.org/project/jsonapi/issues/2971040

CommentFileSizeAuthor
#6 3216613-6_rename-resource-reserved-keyword.patch15.41 KBptmkenny
#2 3216613-2_rename-resource-reserved-keyword.patch12.51 KBptmkenny

Comments

fadonascimento created an issue. See original summary.

ptmkenny’s picture

ptmkenny commented
Status: Active » Needs review
StatusFileSize
new 3216613-2_rename-resource-reserved-keyword.patch12.51 KB

Basic patch doing the rename in the same way as JSON:API module (Resource to JsonApiResource).

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs review » Postponed (maintainer needs more info)

https://www.php.net/manual/en/reserved.other-reserved-words.php

It's not listed anymore and PHPCS wasn't flagging this on DrupalCI.

build:
  assessment:
    validate_codebase:
      phpcs:
        # phpcs will use core's specified version of Coder.
        sniff-all-files: true
        halt-on-fail: true

How did you get this error?

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Postponed (maintainer needs more info) » Needs review

I can't read that page. It's there on the second table. I still don't get why this didn't fail at all before.

Status: Needs review » Needs work

The last submitted patch, 2: 3216613-2_rename-resource-reserved-keyword.patch, failed testing. View results

ptmkenny’s picture

ptmkenny commented
Status: Needs work » Needs review
StatusFileSize
new 3216613-6_rename-resource-reserved-keyword.patch15.41 KB

Fixing the tests.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Actually, List of other reserved words says it'ss highly discouraged to use resource to name to name a class, interface or trait, or in namespaces. It's not an issue with current PHP versions, including PHP 8.1, but future PHP versions could create issues with classes named Resource.

Changing a class name or it's namespace isn't a BC change. It should probably be done in a different branch.

As side note, also Drupal core has a Resource class. I would look at what Drupal core does to fix that.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

For Drupal core, the issue is #2990879: 'resource' is a soft reserved keyword as of PHP version 7.0 and should not be used to name a class, interface or trait or as part of a namespace.

ptmkenny’s picture

ptmkenny commented

Yes, this definitely breaks BC and requires a new major version, especially because the whole point of this module is to create your own resources, all of which will have to be updated. So when this change is made, there should be a very clear note stating that all Resource classes need to be renamed JsonApiResource.

@apaderno Thanks for pointing to the core issue. In this case, I think the most relevant issue is the JSON:API issue referenced in the issue summary, as this module is extending JSON:API, so we should follow its lead (and, it is now part of core).

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

I think this should just be Postponed until we have reason to make a 2.x branch. One of those reasons being resource is no longer soft reserved or we make some of the "unstable" namespace stable, or something.

Not going to immediately move to postponed, waiting for feedback. If you'll be at DrupalCon, let's discuss!

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

I think that posting this issue is fine. I guess that, by the time a new branch will be necessary, PHP won't still throw a warning or an exception for a class named Resource.

ptmkenny’s picture

ptmkenny commented

I would argue that this should be done sooner rather than later because JSON:API in core has already made the change, so this module should aim to be as consistent as possible with the naming already in use in core.

This is especially the case because JSON:API is a great starting point for non-drupal developers to jump into Drupal, but finding things like inconsistently named classes and having to search through issue queues to figure out what's up is not good DX.

Also, the linked JSON:API issue marked this as critical-- whether that was warranted I am not qualified to say, but it would be a bit strange for one module to address an issue as critical and another module to postpone the same fix indefinitely.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented

I think it was marked as critical for Drupal core. This effects about 6 or 7 contrib which extend this module. There is nothing forcing us to do this now. It was just a minor oversight.

I'm not a fan of making new versions just to make new versions. I understand it may or will happen. And that's okay. We'll have plenty of heads up whenever PHP makes the announcement. In the meantime, we could open issues in each contrib marked as "Postponed" with this as a parent.

Each of those modules will then need a major version release. It's painful, and that pain is not warranted right now.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Actually, #2971040: PHP 7.1 Compatibility Warning is for a contributed module that became part of Drupal core after that issue was fixed. If we are going to look at what Drupal core does, the right issue is #2990879: 'resource' is a soft reserved keyword as of PHP version 7.0 and should not be used to name a class, interface or trait or as part of a namespace, which is still not fixed after 4 years.

In that issue, though, the change would involve two traits that have been marked as internal, an entity class (which is considered internal because it's a plugin), and a test class (which is considered internal too). It's not a change that involves a class contributed modules can freely use, or Drupal core would make the change in Drupal 10. In the same way, this change (which involves a class used by other contributed modules) should be done in a new branch.

ptmkenny’s picture

ptmkenny commented

Thanks for the additional clarification.

I now see why it makes sense to postpone this. I appreciate the two of you taking the time to explain.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs review » Closed (won't fix)

Marking as "won't fix" per this RFC https://wiki.php.net/rfc/namespaced_names_as_token

With PHP 8 namespaces are one token instead of multiple. So using soft reserved words is not a problem.