This module provides a field type analogous to Drupal's core entity reference but references entities in other Drupal systems via JSON:API.

An introductory video to demonstrate the use of the module is available here.

Project link

https://www.drupal.org/project/jsonapi_reference

Comments

Eli-T created an issue. See original summary.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue summary: View changes
shashank5563’s picture

shashank5563 commented

Thank you for applying! Reviewers will review the project files, describing what needs to be changed.

Please read Review process for security advisory coverage: What to expect for more details and Security advisory coverage application checklist to understand what reviewers look for. Tips for ensuring a smooth review gives some hints for a smoother review.

To reviewers: Please read How to review security advisory coverage applications, What to cover in an application review, and Drupal.org security advisory coverage application workflow.

While this application is open, only the user who opened the application can make commits to the project used for the application.

Reviewers only describe what needs to be changed; they don't provide patches to fix what reported in a review.

shashank5563’s picture

shashank5563 commented

Fix PHPCS Issue.

vendor/bin/phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,php  web/modules/contrib/jsonapi_reference/


FILE: //web/modules/contrib/jsonapi_reference/jsonapi_reference.post_update.php
------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------
  1 | ERROR   | [x] End of line character is invalid; expected "\n" but found "\r\n"
 26 | WARNING | [ ] Unused variable $article_node_type.
------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------
shashank5563’s picture

shashank5563 commented
Status: Needs review » Needs work
eli-t’s picture

eli-t
he/him
Primary language English
Location Manchester
commented
Status: Needs work » Needs review

Thank you @shashank5563 ❤️

I have run PHPCS and only receive one warning:

jsonapi_reference on  2.0.x via 🐘 v8.1.13 on ☁️  (eu-west-1)
❯ ddev exec vendor/bin/phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,php  web/modules/contrib/jsonapi_reference/

FILE: /var/www/html/web/modules/contrib/jsonapi_reference/jsonapi_reference.post_update.php
-------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------------
 26 | WARNING | Unused variable $article_node_type.
-------------------------------------------------------------------------------------------

Have you definitely reviewed 2.0.x? Is there any chance you may have introduced that change of line ending locally?

I have checked out a fresh version of that branch that has not been opened in an IDE (so could not have been auto-corrected) and have verified the line endings are correct and identical:

jsonapi_reference on  2.0.x via 🐘 v8.1.13 on ☁️  (eu-west-1)
❯ git ls-files --eol
i/lf    w/lf    attr/                 	.gitignore
i/lf    w/lf    attr/                 	README.txt
i/lf    w/lf    attr/                 	composer.json
i/lf    w/lf    attr/                 	config/schema/jsonapi_reference.schema.yml
i/lf    w/lf    attr/                 	jsonapi_reference.info.yml
i/lf    w/lf    attr/                 	jsonapi_reference.links.menu.yml
i/lf    w/lf    attr/                 	jsonapi_reference.module
i/lf    w/lf    attr/                 	jsonapi_reference.permissions.yml
i/lf    w/lf    attr/                 	jsonapi_reference.post_update.php
i/lf    w/lf    attr/                 	jsonapi_reference.routing.yml
i/lf    w/lf    attr/                 	jsonapi_reference.services.yml
i/lf    w/lf    attr/                 	src/Controller/TypedResourceController.php
i/lf    w/lf    attr/                 	src/Form/JsonApiReferenceConfigForm.php
i/lf    w/lf    attr/                 	src/JsonApiClient.php
i/lf    w/lf    attr/                 	src/JsonApiClientInterface.php
i/lf    w/lf    attr/                 	src/Plugin/Field/FieldFormatter/TypedResourceObjectStringFormatter.php
i/lf    w/lf    attr/                 	src/Plugin/Field/FieldType/TypedResourceObjectItem.php
i/lf    w/lf    attr/                 	src/Plugin/Field/FieldWidget/TypedResourceObjectAutocompleteWidget.php
i/lf    w/lf    attr/                 	tests/src/Functional/JsonApiReferenceFieldBase.php
i/lf    w/lf    attr/                 	tests/src/Functional/JsonApiReferenceFieldFormatterTest.php
i/lf    w/lf    attr/                 	tests/src/Functional/JsonApiReferenceTest.php
i/lf    w/lf    attr/                 	tests/src/Unit/UuidTest.php
shashank5563’s picture

shashank5563 commented

@Eli-T, The new line doesn't have matter because, this is my PHPC settings. Have you fixed the second point?

Unused variable $article_node_type

eli-t’s picture

eli-t
he/him
Primary language English
Location Manchester
commented

@shashank5563 I have just fixed that on commit c6a6d4b3fab9ef16df1a2daf4a825043fe93f695 just pushed to HEAD of 2.0.x.

kevin.brocatus’s picture

kevin.brocatus commented

Ran PHPCS and no issues were found. Also went through the code and I have to say the code looks very clean and well-written. I have a few comments.

First of all, my IDE (PHPStorm in this case) does not like consecutive dots in comments in the TypedResourceObjectAutocompleteWidget class. I don't necessarily think it's an issue, but might be worth changing.

In the class doc of TypedResourceObjectStringFormatter, I think you want to change the following;

Plugin implementation of a 'basic_string' formatter.

to

Plugin implementation of a 'typed_resource_object_string' formatter.

I also see that you define your return types in every class except for the TypedResourceObjectStringFormatter class. I recommend adding those for consistency.

You are also missing the return type of the create function in the TypedResourceObjectStringFormatter class.

vinaymahale’s picture

vinaymahale commented
Issue summary: View changes
eli-t’s picture

eli-t
he/him
Primary language English
Location Manchester
commented
Status: Needs review » Needs work

Thanks @kevin.brocatus!

I have added return types to TypedResourceObjectStringFormatter as suggested and updated the class doc of the same, addressing two of your points in #3375840-9: [2.0.x] JSON:API Reference

I have not removed the consecutive dots in the comments. I don't know what specific issue they cause you in PHPStorm, but I also use PHPStorm and such style of comments does not cause any problems in my instance. If I'm the outlier then maybe it would be worth changing, but I feel this style of comment useful, for example when you want to use a two comments to explain a condition and action with two comments that gramatically consist of a single sentence.

These changes are in commit 648a03bdc6858d1b585d5e9bd8435cd1d7e356b6 on 2.0.x

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue summary: View changes
eli-t’s picture

eli-t
he/him
Primary language English
Location Manchester
commented
Status: Needs work » Needs review

Putting back to needs review as comments are addressed

shashank5563’s picture

shashank5563 commented

@Eli-T I have reviewed the changes, and they look fine to me.

Let’s wait for other reviewers to take a look and if everything goes fine, you will get the role.

vinaymahale’s picture

vinaymahale commented
Priority: Normal » Major

I am changing priority as per Issue priorities.

vinaymahale’s picture

vinaymahale commented
Priority: Major » Critical
vinaymahale’s picture

vinaymahale commented
Priority: Critical » Normal
Status: Needs review » Reviewed & tested by the community

No issues found
Changing status to RTBC!

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: Unassigned » avpaderno

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the Slack #contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the reviewers.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Reviewed & tested by the community » Fixed
eli-t’s picture

eli-t
he/him
Primary language English
Location Manchester
commented

Thank you @apaderno, @vinaymahale, @shashank5563, and @kevin.brocatus for your help 💙

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.