Problem/Motivation

To make it easier for other developers to contribute code, let's update Views RSS to follow the Drupal coding standards.

Proposed resolution

Update the code to follow Drupal coding standards

Remaining tasks

  1. Write a patch
  2. Review
  3. Commit

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#31 views_rss-n3306849-31.interdiff.txt485 bytesdamienmckenna
#31 views_rss-n3306849-31.patch15.53 KBdamienmckenna
#30 views_rss-n3306849-30.interdiff.txt2.78 KBdamienmckenna
#30 views_rss-n3306849-30.patch15.05 KBdamienmckenna
#29 views_rss-n3306849-28.patch19.25 KBdamienmckenna
#29 views_rss-n3306849-28.interdiff.txt4.9 KBdamienmckenna
#26 views_rss-n3306849-26.patch17.18 KBdamienmckenna
#26 views_rss-n3306849-26.interdiff.txt2.95 KBdamienmckenna
#23 interdiff_18-23.txt3.54 KBadaucyj
#23 3306849-23.patch17.09 KBadaucyj
#18 3306849-18.patch14.41 KBjuancec
#11 3306849-4.patch14.1 KBLeoAlcci
#3 3306849-3.patch12.96 KBlucasbaralm

Issue fork views_rss-3306849

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

idebr created an issue. See original summary.

lucasbaralm’s picture

lucasbaralm
He/Him
commented
Assigned: Unassigned » lucasbaralm

I will work on this!

lucasbaralm’s picture

lucasbaralm
He/Him
commented
StatusFileSize
new 3306849-3.patch12.96 KB

Running phpcs I found the following standards errors:

FILE: views_rss/tests/src/Functional/DisplayFeedTest.php
-----------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
-----------------------------------------------------------------------------------------------------------
   9 | WARNING | [x] Unused use statement
 148 | WARNING | [x] There must be no blank line following an inline comment
 188 | WARNING | [ ] Line exceeds 80 characters; contains 86 characters
 200 | WARNING | [x] There must be no blank line following an inline comment
-----------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------


FILE: views_rss/modules/views_rss_core/views_rss_core.module
---------------------------------------------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
---------------------------------------------------------------------------------------------------------------
  24 | ERROR | [x] Short array syntax must be used to define arrays
 180 | ERROR | [x] Short array syntax must be used to define arrays
 182 | ERROR | [x] Short array syntax must be used to define arrays
---------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------------------


FILE: views_rss/src/Plugin/views/row/RssFields.php
-----------------------------------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 6 LINES
-----------------------------------------------------------------------------------------------------
  41 | ERROR | [x] list(...) is forbidden, use [...] instead.
  65 | ERROR | [x] list(...) is forbidden, use [...] instead.
  88 | ERROR | [x] Use null coalesce operator instead of ternary operator.
  89 | ERROR | [x] Use null coalesce operator instead of ternary operator.
 177 | ERROR | [x] list(...) is forbidden, use [...] instead.
 337 | ERROR | [x] Use null coalesce operator instead of ternary operator.
-----------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 6 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------


FILE: views_rss/src/Plugin/views/style/RssFields.php
-------------------------------------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
-------------------------------------------------------------------------------------------------------
 107 | ERROR | [x] list(...) is forbidden, use [...] instead.
 141 | ERROR | [x] list(...) is forbidden, use [...] instead.
 161 | ERROR | [x] Use null coalesce operator instead of ternary operator.
 162 | ERROR | [x] Use null coalesce operator instead of ternary operator.
 310 | ERROR | [x] list(...) is forbidden, use [...] instead.
-------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------------


FILE: views_rss/views_rss.module
-----------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
-----------------------------------------------------------------------------------
 64 | ERROR | [x] list(...) is forbidden, use [...] instead.
 72 | ERROR | [x] list(...) is forbidden, use [...] instead.
-----------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------

Time: 219ms; Memory: 12MB

I fixed all but this one:


FILE: /home/lucasba/Documents/9.5/drupal/modules/contrib/views_rss/tests/src/Functional/DisplayFeedTest.php
-----------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------
 186 | WARNING | Line exceeds 80 characters; contains 86 characters
-----------------------------------------------------------------------------------------------------------

It is relative to a comment that, for readability, I believe it is better not to break the line:

// <cloud domain="www.example.com" path="/viewsrsscloud.html" protocol="https"/>\n

lucasbaralm’s picture

lucasbaralm
He/Him
commented
Assigned: lucasbaralm » Unassigned
lucasbaralm’s picture

lucasbaralm
He/Him
commented
Status: Active » Needs review

Sorry i forgot to move to Needs review.

urvashi_vora’s picture

urvashi_vora commented
Assigned: Unassigned » urvashi_vora

Hi,

I will review this

urvashi_vora’s picture

urvashi_vora commented

Hi,

I reviewed your patch.

urvasi@urvasi-Inspiron-15-3552:/var/www/html/contribution/d8_cont/web/modules/contrib/views_rss-3306849$ git apply -v 3306849-3.patch
Checking patch modules/views_rss_core/views_rss_core.module...
Checking patch src/Plugin/views/row/RssFields.php...
Checking patch src/Plugin/views/style/RssFields.php...
Checking patch tests/src/Functional/DisplayFeedTest.php...
Checking patch views_rss.module...
Applied patch modules/views_rss_core/views_rss_core.module cleanly.
Applied patch src/Plugin/views/row/RssFields.php cleanly.
Applied patch src/Plugin/views/style/RssFields.php cleanly.
Applied patch tests/src/Functional/DisplayFeedTest.php cleanly.
Applied patch views_rss.module cleanly.
urvashi_vora’s picture

urvashi_vora commented
Assigned: urvashi_vora » Unassigned
Status: Needs review » Needs work

Still, there are some issues left, hence moving it to needs work.

urvasi@urvasi-Inspiron-15-3552:/var/www/html/contribution/d8_cont/web/modules/contrib$ phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig views_rss-3306849/

FILE: ...modules/contrib/views_rss-3306849/tests/src/Functional/DisplayFeedTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AND 2 WARNINGS AFFECTING 3 LINES
--------------------------------------------------------------------------------
  20 | ERROR   | Do not disable strict config schema checking in tests. Instead
     |         | ensure your module properly declares its schema for
     |         | configurations.
 141 | WARNING | Unused variable $node_link.
 186 | WARNING | Line exceeds 80 characters; contains 86 characters
--------------------------------------------------------------------------------


FILE: ...ontribution/d8_cont/web/modules/contrib/views_rss-3306849/views_rss.module
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------
 64 | WARNING | Unused variable $element_name.
 72 | WARNING | Unused variable $element_name.
--------------------------------------------------------------------------------

Time: 2.13 secs; Memory: 12MB

urvasi@urvasi-Inspiron-15-3552:/var/www/html/contribution/d8_cont/web/modules/contrib$

Thanks

LeoAlcci’s picture

LeoAlcci commented

I'll work on it!

LeoAlcci’s picture

LeoAlcci commented
Assigned: Unassigned » LeoAlcci
LeoAlcci’s picture

LeoAlcci commented
StatusFileSize
new 3306849-4.patch14.1 KB

I accomplished to resolved almost all the phpcs errors, the only left is:

FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------------------------------
20 | ERROR | Do not disable strict config schema checking in tests. Instead ensure your module properly declares its schema for configurations.
-------------------------------------------------------------------------------------------------------------------------------------------------

Time: 443ms; Memory: 12MB

Please kindly review it!

LeoAlcci’s picture

LeoAlcci commented
Assigned: LeoAlcci » Unassigned
Status: Needs work » Needs review
christyanpaim’s picture

christyanpaim commented
Assigned: Unassigned » christyanpaim
christyanpaim’s picture

christyanpaim commented
Assigned: christyanpaim » Unassigned
gersonjl’s picture

gersonjl
He/Him
Primary language Portuguese, Brazil
commented
Assigned: Unassigned » gersonjl

Will review it

gersonjl’s picture

gersonjl
He/Him
Primary language Portuguese, Brazil
commented
Status: Needs review » Needs work

As said on #11, there is only one error left, will try to fix it

gersonjl’s picture

gersonjl
He/Him
Primary language Portuguese, Brazil
commented
Assigned: gersonjl » Unassigned

Couldn't fix it.

juancec’s picture

juancec commented
Status: Needs work » Needs review
StatusFileSize
new 3306849-18.patch14.41 KB

Hi, I'm providing an updated patch which fixes the PHPCS issue in #11. Kindly review it and let me know your feedback. Good job everyone.

Status: Needs review » Needs work

The last submitted patch, 18: 3306849-18.patch, failed testing. View results

lbonfioli’s picture

lbonfioli commented
Assigned: Unassigned » lbonfioli
Status: Needs work » Active

I'll try to fix the remaining problems.

lbonfioli’s picture

lbonfioli commented
Assigned: lbonfioli » Unassigned
Status: Active » Needs work
adaucyj’s picture

adaucyj
He/Him
Primary language Portuguese, Brazil
Location Irupi, ES
commented
Assigned: Unassigned » adaucyj

I'll work on it.

adaucyj’s picture

adaucyj
He/Him
Primary language Portuguese, Brazil
Location Irupi, ES
commented
Assigned: adaucyj » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3306849-23.patch17.09 KB
new interdiff_18-23.txt3.54 KB

Create schema for tests. Could someone review it, please?

gabrieldv’s picture

gabrieldv commented
Assigned: Unassigned » gabrieldv

I'll be reviewing it, thanks you all for all the work with the patches :)

gabrieldv’s picture

gabrieldv commented
Assigned: gabrieldv » Unassigned
Status: Needs review » Reviewed & tested by the community

Tested the #23 patch and phpcs returned no error. Also tested in the automated tests and installing the module in CMS: testing a bit, nothing wrong, so i'm moving it to RTBC, again great work.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Parent issue: » #2761449: Plan for Advanced Views RSS Feed 8.x-2.0 (for Drupal 9/10)
StatusFileSize
new views_rss-n3306849-26.interdiff.txt2.95 KB
new views_rss-n3306849-26.patch17.18 KB

I made some further improvements, and one of the changes was already committed in a separate issue.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: views_rss-n3306849-26.patch, failed testing. View results

idebr’s picture

idebr commented

tests/modules/views_rss_test_config/config/schema/views_rss.style.schema.yml is redundant with the config schema added in #3313908: Implement configuration schema and can be removed.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs work » Needs review
StatusFileSize
new views_rss-n3306849-28.interdiff.txt4.9 KB
new views_rss-n3306849-28.patch19.25 KB

Hopefully this fixes the last issue.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
StatusFileSize
new views_rss-n3306849-30.patch15.05 KB
new views_rss-n3306849-30.interdiff.txt2.78 KB

@idebr: Yeah, thanks, I was coming to the same conclusion.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
StatusFileSize
new views_rss-n3306849-31.patch15.53 KB
new views_rss-n3306849-31.interdiff.txt485 bytes

The test submodule depends on views_rss_core, not just Views.

  • DamienMcKenna committed dcde596e on 8.x-2.x 
    Issue #3306849 by DamienMcKenna, adaucyj, lucasbaralm, LeoAlcci,...
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs review » Fixed

Committed. Thanks everyone!

Status: Fixed » Closed (fixed)

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