Part of meta-issue #1518116: [meta] Make Core pass Coder Review

  • Aggregator module fails on the Drupal 8 branch test code review tab. The failures are itemized on that tab.
  • Drupal Code Sniffer reports three errors with the latest patch here applied:
    FILE: ...quickstart/websites/d8.dev/core/modules/aggregator/aggregator.admin.inc
    --------------------------------------------------------------------------------
    FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
    --------------------------------------------------------------------------------
     508 | ERROR | Use XHTML style <br /> tags instead of <br>
     630 | ERROR | If the line declaring an array spans longer than 80 characters,
         |       | each element should be broken into its own line
    --------------------------------------------------------------------------------
    
    
    FILE: ...uickstart/websites/d8.dev/core/modules/aggregator/aggregator.parser.inc
    --------------------------------------------------------------------------------
    FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
    --------------------------------------------------------------------------------
     336 | ERROR | There should be no white space after an opening "("
    --------------------------------------------------------------------------------

    All three are considered to be false positives in this case:

  • Coder Review finds no problems at the minor (strictest) severity warning level.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TravisCarden’s picture

Assigned: TravisCarden » Unassigned
Status: Active » Needs review
FileSize
38.17 KB
TravisCarden’s picture

FileSize
38.17 KB

Oops. I put the wrong issue number in the commit message.

oriol_e9g’s picture

Great work!!! Two points for the discusion!

1) I don't like this change:

-    list($year, $month, $day, $hours, $minutes, $seconds) = array($match[1], $match[2], $match[3], $match[4], $match[5], $match[6]);
+    list(, $year, $month, $day, $hours, $minutes, $seconds) = $match;

I think that this should be: list(NULL, $year, $month, $day, $hours, $minutes, $seconds) or revert the change.

2) Do we really need to expand arrays when we define a function?

+function aggregator_form_category($form, &$form_state, $edit = array(
+  'title' => '',
+  'description' => '',
+  'cid' => NULL,
+)) {

I know that coding standard says that we must expand arrays when they exceeds 80 characters, but I don't see that this change in this case adds readability to the code.

TravisCarden’s picture

Thanks, @oriol_e9g.

1) Using NULL causes a parse error for me. The PHP manual suggests there should just be a space before the comma. Do you like that any better?

list( , $year, $month, $day, $hours, $minutes, $seconds) = $match;

I did this to promote conciseness and readability. Otherwise we'd need to break the array onto multiple lines to conform to the coding standards.

2) I'm happy to submit to the consensus on this one. I just "fixed" whatever the Drupal Code Sniffer complained about. :) Does everyone share your feelings on this?

Lars Toomre’s picture

Status: Needs review » Needs work
+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -40,9 +40,12 @@ function aggregator_page_source($feed) {
+ * @return string
+ *   The rendered list of items for the feed.

Since this is a _form() function, @return directive should not be supplied.

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -54,9 +57,12 @@ function aggregator_page_source_form($form, $form_state, $feed) {
+ * @return string
+ *   The rendered list of items for the category.

Same here; no @return directive needed for a _form() function.

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -73,9 +79,12 @@ function aggregator_page_category($category) {
+ * @return string
+ *   The rendered list of items for the category.

Ibid.

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -87,13 +96,13 @@ function aggregator_page_category_form($form, $form_state, $category) {
  *   The type of filter for the items. Possible values are:
  *   - sum: No filtering.
  *   - source: Filter the feed items, limiting the result to items from a
  *     single source.

Since these are specific string values (rather than possible keys to an array), I believe our documentation standard calls for each of these to be enclosed in a single quote (ie - 'sum':). Double check please.

+++ b/core/modules/aggregator/aggregator.processor.incundefined
@@ -180,7 +217,7 @@ function aggregator_save_item($edit) {
  *   Object describing feed.

This description could be improved to explain what is expected to be in the object.

TravisCarden’s picture

Status: Needs work » Needs review
FileSize
38.31 KB

Thanks, @Lars Toomre! Here's an updated patch. I removed the three extraneous @returns. I haven't found a clear policy on quoting strings, but there is one example of double quoting on the Doxygen and comment formatting conventions handbook page, so I opted to take that route. I also expanded the description of the $feed object. (If someone has a better description in mind, I welcome it!)

xjm’s picture

Status: Needs review » Needs work

A lot of the array reformatting in this patch makes the arrays harder to read. I especially don't like stuff like this:

+++ b/core/modules/aggregator/aggregator.admin.incundefined
@@ -67,10 +89,27 @@ function aggregator_view() {
+    900,
+    1800,
+    3600,
+    7200,
+    10800,
+    21600,
+    32400,
+    43200,
+    64800,
+    86400,
+    172800,
+    259200,
+    604800,
+    1209600,
+    2419200,
+  ), 'format_interval');

The final argument gets "lost" with this formatting, and I actually find the list of numbers easier to read on one line. If we use a multiline format, each parameter should also be on its own line(s).

+++ b/core/modules/aggregator/aggregator.parser.incundefined
@@ -137,7 +146,18 @@ function aggregator_parse_feed(&$data, $feed) {
+    foreach (array(
+      'pubdate',
+      'dc:date',
+      'dcterms:issued',
+      'dcterms:created',
+      'dcterms:modified',
+      'issued',
+      'created',
+      'modified',
+      'published',
+      'updated',
+    ) as $key) {

foreach is a language construct, not a function, and so I think our coding standards dictate that it should be on one line. (I am extrapolating from what it says about conditions at http://drupal.org/coding-standards#linelength.) If we really want to make the array more legible, define it up a line and then foreach ($feed_whatevermacallums as $key) {}.

TravisCarden’s picture

Status: Needs work » Needs review
FileSize
38.27 KB

You're right @xjm. Here. I think you'll like this patch better.

TravisCarden’s picture

Issue tags: +Novice, +coder-fixes-2012

Adding tags.

Status: Needs review » Needs work

The last submitted patch, drupal-1512434-8.patch, failed testing.

xjm’s picture

Agreed, the reformatting you've done here makes the code much easier to read. The addition of range() is also a nice improvement. Couple remaining thoughts:

  1. +++ b/core/modules/aggregator/aggregator.admin.incundefined
    @@ -67,10 +91,28 @@ function aggregator_view() {
    +  $period = array(
    ...
    +  $period = drupal_map_assoc($period, 'format_interval');
    
    @@ -249,7 +304,24 @@ function aggregator_admin_remove_feed_submit($form, &$form_state) {
    +  $period = array(
    ...
    +  $period = drupal_map_assoc($preiod, 'format_interval');
    
    +++ b/core/modules/aggregator/aggregator.processor.incundefined
    @@ -73,7 +82,22 @@ function aggregator_form_aggregator_admin_form_alter(&$form, $form_state) {
    +    $period = array(
    ...
    +    $period = drupal_map_assoc($period, 'format_interval');
    

    Still not quite sure about these; it's sort of confusing to have the variable's content laid out like that but then redefine it as an associated array on the following line. (Also, the fact that we have this three times makes me wonder if we should have an aggregator_intervals() or something, but that is perhaps out of scope.)

    Also, there is a typo in the second one ($preiod) making those tests fail. (Yay testbot doing its job!)

  2. +++ b/core/modules/aggregator/aggregator.admin.incundefined
    @@ -145,10 +191,19 @@ function aggregator_form_feed_validate($form, &$form_state) {
    -      $result = db_query("SELECT title, url FROM {aggregator_feed} WHERE (title = :title OR url = :url) AND fid <> :fid", array(':title' => $form_state['values']['title'], ':url' => $form_state['values']['url'], ':fid' => $form_state['values']['fid']));
    +      $args = array(
    +        ':title' => $form_state['values']['title'],
    +        ':url' => $form_state['values']['url'],
    +        ':fid' => $form_state['values']['fid'],
    +      );
    +      $result = db_query("SELECT title, url FROM {aggregator_feed} WHERE (title = :title OR url = :url) AND fid <> :fid", $args);
    

    This is definitely an improvement. I might format this like:

    $result = db_query(
      "SELECT title, url FROM {aggregator_feed} WHERE (title = :title OR url = :url) AND fid <> :fid",
      array(
        ':title' => $form_state['values']['title'],
        ':url' => $form_state['values']['url'],
        ':fid' => $form_state['values']['fid'],
      ),
    );
    

    That's a pattern we use elsewhere in core.

  3. +++ b/core/modules/aggregator/aggregator.pages.incundefined
    @@ -87,22 +87,22 @@ function aggregator_page_category_form($form, $form_state, $category) {
    + *   - "sum": No filtering.
    + *   - "source": Filter the feed items, limiting the result to items from a
    ...
    + *   - "category": Filter the feed items by category.
    ...
    + *   - "source": $data is an object with $data->fid identifying the feed used to
    

    I personally think it's confusing to differentiate between string value literals and array keys in lists (since array keys are, after all, string literals anyway). I'd prefer to just consistently use the standard in http://drupal.org/node/1354#lists so that there isn't any confusion about what to use when. (I think I recommended differently before five months of API doc cleanups, though, and I think jhodgdon may still disagree on this point.)

  4. +++ b/core/modules/aggregator/aggregator.parser.incundefined
    @@ -147,7 +170,8 @@ function aggregator_parse_feed(&$data, $feed) {
    +      // Aggregator_parse_w3cdtf() returns FALSE on failure.
    

    Pretty sure the function name does not start with a capital letter, so let's fix that too. :)

  5. +++ b/core/modules/aggregator/aggregator.parser.incundefined
    @@ -294,18 +330,19 @@ function aggregator_element_data($parser, $data) {
    +    // Z is zulu time, aka GMT.
    

    While we are fixing this comment, let's make a couple of grammatical corrections:
    // Z is Zulu time, which is the same as GMT.

Thanks!

TravisCarden’s picture

FileSize
38.22 KB

@xjm:

  1. I agree it's not good to duplicate the interval code. I think an aggregator_intervals() function would be a good idea. If there's consensus I'll implement it; just tell me where it should go. (I don't perceive an obvious pattern in how the functions are ordered in their files—they're not alphabetical, for example.) Abstracting the intervals code, however, won't solve the problem of formatting it. Would someone like to propose a better arrangement? ... I fixed the mistyped variable name.
  2. I've adopted your suggestion for formatting db_query().
  3. I agree with you: it's best to have one, simple-to-apply standard for formatting lists. I've reverted the string quoting.
  4. I fixed the capitalized function name...
  5. ...and made your grammatical corrections to the Zulu time comment.

Here's an updated patch.

TravisCarden’s picture

Status: Needs work » Needs review
xjm’s picture

FileSize
5.25 KB

By the way, it can be helpful to include an interdiff with your patch that shows the changes you've made from the previous version. Here's one between #8 and #12.

xjm’s picture

Those changes look good to me, thanks.

For the $period listings I think the formatting itself complies with our standard; it's just weird to have the same fat list of magic numbers repeated with no explanation of what it means. Presumably they're time intervals for sensible numbers of second like "30 minutes" to "1 week" but nothing in the code indicates that. I changed my mind about aggregator_intervals() because Drupal does this lists-of-times thing all over the place, not just in this module.

However, it's out of scope for a code cleanup, so let's punt that question for now. :) If you like, you could open a followup issue to investigate all the places core has lists like this and maybe standardize 'em with an API function. (Or maybe there is one already and aggregator just isn't using it?)

Two last comments on the latest patch:

  1. +++ b/core/modules/aggregator/aggregator.admin.incundefined
    @@ -38,7 +44,14 @@ function aggregator_view() {
    -  $output .= theme('table', array('header' => $header, 'rows' => $rows, 'empty' => t('No feeds available. <a href="@link">Add feed</a>.', array('@link' => url('admin/config/services/aggregator/add/feed')))));
    +  $table = array(
    +    'header' => $header,
    +    'rows' => $rows,
    +    'empty' => t('No feeds available. <a href="@link">Add feed</a>.', array(
    +      '@link' => url('admin/config/services/aggregator/add/feed'),
    +    )),
    +  );
    +  $output .= theme('table', $table);
    
    @@ -47,9 +60,20 @@ function aggregator_view() {
    -  $output .= theme('table', array('header' => $header, 'rows' => $rows, 'empty' => t('No categories available. <a href="@link">Add category</a>.', array('@link' => url('admin/config/services/aggregator/add/category')))));
    +  $table = array(
    +    'header' => $header,
    +    'rows' => $rows,
    +    'empty' => t('No categories available. <a href="@link">Add category</a>.', array(
    +      '@link' => url('admin/config/services/aggregator/add/category'),
    +    )),
    +  );
    +  $output .= theme('table', $table);
    
    +++ b/core/modules/aggregator/aggregator.pages.incundefined
    @@ -282,7 +284,11 @@ function theme_aggregator_categorize_items($variables) {
    -  $output .= theme('table', array('header' => array('', t('Categorize')), 'rows' => $rows));
    +  $table = array(
    +    'header' => array('', t('Categorize')),
    +    'rows' => $rows,
    +  );
    +  $output .= theme('table', $table);
    
    @@ -559,7 +562,11 @@ function template_preprocess_aggregator_summary_item(&$variables) {
    -  $variables['source_icon'] = theme('feed_icon', array('url' => $feed->url, 'title' => t('!title feed', array('!title' => $feed->title))));
    +  $feed_icon_variables = array(
    +    'url' => $feed->url,
    +    'title' => t('!title feed', array('!title' => $feed->title)),
    +  );
    +  $variables['source_icon'] = theme('feed_icon', $feed_icon_variables);
    

    We could probably use a similar formatting for theming output as we do for the queries. What do you think?

  2. +++ b/core/modules/aggregator/aggregator.parser.incundefined
    @@ -79,8 +79,18 @@ function aggregator_parse_feed(&$data, $feed) {
    +    $variables = array(
    +      '%site' => $feed->title,
    +      '%error' => xml_error_string(xml_get_error_code($xml_parser)),
    +      '%line' => xml_get_current_line_number($xml_parser),
    +    );
    +    watchdog('aggregator', 'The feed from %site seems to be broken due to an error "%error" on line %line.', $variables, WATCHDOG_WARNING);
    +    $options = array(
    +      '%site' => $feed->title,
    +      '%error' => xml_error_string(xml_get_error_code($xml_parser)),
    +      '%line' => xml_get_current_line_number($xml_parser),
    +    );
    +    drupal_set_message(t('The feed from %site seems to be broken because of error "%error" on line %line.', $options), 'error');
    

    Aren't $variables and $options the same here? Can we just reuse it? Maybe call it $message_variables. (The string is also the same but unfortunately needs to be duplicated as a literal because watchdog assembles the string rather than expecting a string assembled with t().)

Aside from those last two questions, the patch in #12 looks ready to me. Thanks @TravisCarden!

TravisCarden’s picture

FileSize
9.14 KB
37.99 KB

Excellent.

  1. I'm all for using fewer variables if the format is acceptable. Implemented.
  2. You're right: the variables are identical. What if we do this?
    $message = 'The feed from %site seems to be broken due to an error "%error" on line %line.';
    $message_variables = array(
      '%site' => $feed->title,
      '%error' => xml_error_string(xml_get_error_code($xml_parser)),
      '%line' => xml_get_current_line_number($xml_parser),
    );
    watchdog('aggregator', $message, $message_variables, WATCHDOG_WARNING);
    drupal_set_message(t($message, $message_variables), 'error');
    

Here's an updated patch.

xjm’s picture

Status: Needs review » Needs work
+++ b/core/modules/aggregator/aggregator.parser.incundefined
@@ -79,8 +79,14 @@ function aggregator_parse_feed(&$data, $feed) {
+    drupal_set_message(t($message, $message_variables), 'error');

We can't do this, unfortunately. Strings need to be directly inside t() in order to be translated. (We can still reuse the array of arguments, though.)

Looks good to me other than that, though!

TravisCarden’s picture

Status: Needs work » Needs review
FileSize
2.62 KB
38.04 KB

Aw, for sadness. :) Well, here's the patch minus that, then.

TravisCarden’s picture

Marked #1035712: Should follow Coder's best practices. as a duplicate of this issue.

TravisCarden’s picture

Title: Fix coding standards violations in aggregator module » Make Aggregator module pass Coder Review

Updating issue title per initiative standard.

TravisCarden’s picture

Assigned: Unassigned » TravisCarden

Oops. Sorry for the noise: I didn't realize this wasn't assigned to me anymore.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Alright, that looks good to me.

I also scanned through all the added data type documentation in the docblocks, and checked it against the @param/@return description if it was clear or looked up the function on api.d.o if not. They all look correct, but this is something we'll want to keep a close eye on in a review. Easy (and annoying) to get those wrong, and time-consuming to check them in a review.

catch’s picture

#18: drupal-1512434-18.patch queued for re-testing.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Let's update the issue here with:

  • Whether this directory either passes or fails on the Drupal 8 branch test code review tab. If it fails, also note whether the failures are itemized on that page.
  • Whether the directory has been tested with Drupal Code Sniffer with the patch applied, whether it passes or fails, and what settings were used.
  • Whether the directory has been tested with the D8 sandbox of Coder with the patch applied, whether it passes or fails, and what settings were used.

In each case note any failures that were not corrected, and why.

xjm’s picture

Issue summary: View changes

Updated issue summary.

TravisCarden’s picture

Done.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Awesome. Can we also check for/file drupalcs issues for the first two false positives as well?

Meanwhile, RTBC.

xjm’s picture

Issue summary: View changes

Updated issue summary.

TravisCarden’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

There are a couple of changes I'm not sure about here (some discussed above I think)... I think we should limit these patches to strict *coding standards required* changes only, and not make other extraneous changes, right?

a)

-  $output .= theme('table', array('header' => $header, 'rows' => $rows, 'empty' => t('No feeds available. <a href="@link">Add feed</a>.', array('@link' => url('admin/config/services/aggregator/add/feed')))));
+  $output .= theme(
+    'table',
+    array(
+      'header' => $header,
...

I would normally think this should be:

-  $output .= theme('table', array('header' => $header, 'rows' => $rows, 'empty' => t('No feeds available. <a href="@link">Add feed</a>.', array('@link' => url('admin/config/services/aggregator/add/feed')))));
+  $output .= theme('table', array(
+      'header' => $header,
...

I don't see why 'table' and array( need to be on their own lines? This occurs a couple of times in the patch.

b)

-  $period = drupal_map_assoc(array(900, 1800, 3600, 7200, 10800, 21600, 32400, 43200, 64800, 86400, 172800, 259200, 604800, 1209600, 2419200), 'format_interval');
+  $period = array(
+    900,
...
+  $period = drupal_map_assoc($period, 'format_interval');

Agreeing with the review above by xjm, let's leave the drupal_map_assoc() call in there from the beginning.

c)

- * @param $edit
+ * @param array $edit

Is putting types on params/arrays within the scope of this issue? I hope not... It makes the patches much much much harder to verify/review. I have to give the patches a final review before I commit them, and I am really not up for the effort of going through ALL of core Drupal and making sure the param/return types are OK. So can we specifically not have that be part of this meta-issue, and take those parts of the patch out?

d) I'm not excited about this change, kind of like (b):

-    foreach (array('pubdate', 'dc:date', 'dcterms:issued', 'dcterms:created', 'dcterms:modified', 'issued', 'created', 'modified', 'published', 'updated') as $key) {
+    $keys = array(
+      'pubdate',
+      'dc:date',
+      'dcterms:issued',
+      'dcterms:created',
+      'dcterms:modified',
+      'issued',
+      'created',
+      'modified',
+      'published',
+      'updated',
+    );
+    foreach ($keys as $key) {
TravisCarden’s picture

Thank you, @jhodgdon.

  1. So, to confirm, you would change this:
    $output .= theme('table', array('header' => $header, 'rows' => $rows, 'empty' => t('No feeds available. <a href="@link">Add feed</a>.', array('@link' => url('admin/config/services/aggregator/add/feed')))));
    

    to this?

    $output .= theme('table', array(
      'header' => $header,
      'rows' => $rows,
      'empty' => t('No feeds available. <a href="@link">Add feed</a>.', array(
        '@link' => url('admin/config/services/aggregator/add/feed'),
      )),
    ));
    
  2. Re:
    $period = drupal_map_assoc(array(900, 1800, 3600, 7200, 10800, 21600, 32400, 43200, 64800, 86400, 172800, 259200, 604800, 1209600, 2419200), 'format_interval');
    

    The CodeSniffer flags this as an error because the array definition spans more than 80 characters. Is there any way we can respect that standard without making the code so unsightly? Does the standard need some nuancing? In short, how can we more objectively determine when the standard stands and when it doesn't?

  3. The CodeSniffer flags the omission of @param and @return types as errors, which is why we've been adding them. But I don't want to hamper this initiative or make things hard for you. We can remove the changes. We'll just need to amend the initiative instructions. I believe you can specify tests to omit when you run the CodeSniffer. We'll have to look into that and add instructions.

    Do you think we can add the types as part of another initiative for d8? I, personally, find them very helpful, and I'd really like to see them in the next major version.

  4. Re:
    $keys = array(
      'pubdate',
      'dc:date',
      'dcterms:issued',
      'dcterms:created',
      'dcterms:modified',
      'issued',
      'created',
      'modified',
      'published',
      'updated',
    );
    foreach ($keys as $key) {
      // ...
    }
    

    This, like #2, was done to keep the array definition under 80 characters width.

xjm’s picture

I'd actually prefer to make the patches get us all the way to drupalcs compliance so long as they do not directly contradict our coding standards (including adding datatypes if appropriate), because we are likely going to be running drupalcs on the bot in the future.

Regarding the array formatting, I think most of the changes in the patch significantly improve readability and are worthwhile here. The one that's just numbers I could take or leave, but according to our array coding standards it is correct:

Note that if the line declaring an array spans longer than 80 characters (often the case with form and menu declarations), each element should be broken into its own line, and indented one level

Regarding the datatypes, I do think we should be adding them, because they are part of the standard and have been for six months. We just need to get folks submitting patches to also help out by reviewing others' patches, including looking up every API function in each patch, reading it carefully, and confirming that the datatypes cover all the possibilities supported by the function, and then documenting their research explicitly in the issue. Whether we do so in this sprint or separately is I guess worth discussing, but it is kind of weird to tell people "make it pass our coding standards except the datatype one."

jhodgdon’s picture

RE #29 and #28/3 - Yes, can we please please please make the param/return types a separate effort? Really for *all* of core that is a huge effort on the part of the patch makers and patch reviewers to get it right, because you have to read every single function carefully. It's not as simple as "does this code do the same stuff as the previous code". The other changes here can get in easier.

RE #28 -
1. yes, that would be my preference, but coding standards don't specify one or the other. Uses less space.

2. my suggestion would be:

  $period = drupal_map_assoc(array(
    900, 
    1800, 
    ...
    2419200), 'format_interval');

I'm just saying, as was in a review by someone else above, don't split out the drupal_map_assoc() into a separate call.

4. This could be:

 foreach (array(
    'a',
    'b',
    ...'
    'z') as $key) {
   }

On all of these three items, if the consensus is that the patch is better than these suggestions, then that is fine with me. But I'm just saying... it makes the patch easier to review if the changes are all formatting rather than changing the code itself.

NROTC_Webmaster’s picture

I think I finally got drupalcs working properly. My only question is I get the following errors and I'm not sure if they should be corrected or not.

File doc comments must be followed by a blank line. /Drupal/drupal/core/modules/aggregator/aggregator-feed-source.tpl.php line 22 PHP CodeSniffer
File doc comments must be followed by a blank line. /Drupal/drupal/core/modules/aggregator/aggregator-item.tpl.php line 21 PHP CodeSniffer
File doc comments must be followed by a blank line. /Drupal/drupal/core/modules/aggregator/aggregator-summary-items.tpl.php line 19 PHP CodeSniffer
File doc comments must be followed by a blank line. /Drupal/drupal/core/modules/aggregator/aggregator-wrapper.tpl.php line 15 PHP CodeSniffer

xjm’s picture

I think I agree with everything in @jhodgdon's post in #30--let's go ahead and make it explicit that we'll do a separate sprint for adding datatypes--with one exception:

This could be:

 foreach (array(
    'a',
    'b',
    ...'
    'z') as $key) {
   }

On all of these three items, if the consensus is that the patch is better than these suggestions, then that is fine with me. But I'm just saying... it makes the patch easier to review if the changes are all formatting rather than changing the code itself.

I personally find the foreach on multiple lines very confusing, and I think since foreach is a language construct and not a function, a case could be made that that formatting violates:

Conditions should not be wrapped into multiple lines.

jhodgdon’s picture

RE #31 - I think that should be filed as a Code Sniffer bug. We don't normally leave a blank line between the @file and the start of the HTML in templates, and I don't think we want to.

TravisCarden’s picture

NROTC_Webmaster’s picture

Per #31 I will file an issue to document the discrepancy.
I agree the foreach should not be split across multiple lines and if we are going to do another sprint after this to add datatypes to all param/returns then I think we need to specify that in the main summary.

If we have reached a consensus I can also update the summary I just want to make sure everyone is in agreement and its not one of those things that people say we are going to get around to but never actually do.

TravisCarden’s picture

@NROTC_Webmaster, we are all in agreement that we will create a separate initiative to add @param and @return types to dockblocks. If you can get to it before I can, the initiative summary needs to be updated to reflect this fact, and we also need to figure out how to run drupalcs so as to exclude those errors and include the code in the updated summary. I'll try to create a new initiative soon. Thanks!

NROTC_Webmaster’s picture

I'll go ahead and update the issue summary I'm not sure about excluding them from drupalcs. I was barely able to get it to work at all let alone with special exceptions like that.

TravisCarden’s picture

TravisCarden’s picture

Issue summary: View changes

Updated issue summary.

Lars Toomre’s picture

When a patch for this issue next gets re-rolled, we need to check the test class files as well.

I am noticing in many test classes that we are missing a blank line immediately after the 'class ' declaration line and immediately before the closing brace. An example class with this problen is AggregatorConfigurationTest.php.

Also, in other test files, there are members like admin_user. Such cases need to be changed to camel-case names like adminUser.

jhodgdon’s picture

Can you point to where this is in the coding standards? I can't find it... Also, this meta-issue is about "pass Coder review" -- i.e., bringing code up to the standards checked by Coder. Does Coder check for this?

Lars Toomre’s picture

It was present yesterday on http://drupal.org/node/1354#classes in the the notes after the examples. Checking just now, it appears that someone has removed it from there.

Somewhere in the notes from yesterday's work on the API docs, this came up. I will circle back here with a reference when I come across that discussion again (too many recent issues to find which one it was).

jhodgdon’s picture

Right. I removed it yesterday from that section since it is not a documentation-related standard, I didn't know where it came from, and it shouldn't have been in that page. The whitespace guidelines for non-docs sections of code are on http://drupal.org/coding-standards#indenting and they don't include this type of guideline. Nor does the standards page on classes in general http://drupal.org/node/608152 -- so I don't know where that came from and it is not consistent with how most of core is coded.

Anyway, this issue is about *passing coder review*, so unless Coder flags it, we aren't interested in fixing it on this issue at this time.

Lars Toomre’s picture

#1807160: Add missing type hinting to Aggregator module docblocks is open to deal separately with any missing type hinting from @param and/or @return directives.

Lars Toomre’s picture

Status: Postponed » Active
Issue tags: -Novice

I asked the question in #933552: UpperCamelCase is not flagged as error for class definitions about whether improper lowerCamcelCase properties are being flagged for in a Coder Review run. Looking at the file coder_review/includes/coder_review_style.inc, it appears that there only two rules being flagged that pertain to http://drupal.org/node/608152 (Class and method name checks).

In the meantime, I am unable to run Coder for D8. It would be great if someone could produce a sniff report and post it here. Hence, I and/or others can work toward creating a patch that passes whatever now is flagged by Coder (minus the false positive sniffs).

@amitgoyal did just that in the Help module sub-issue #1533244-40: Make help module pass Coder Review that resulted in a Coder review patch being committed there earlier this week. Given that example, I think we can now slowly moved forward with this issue. Hence, moving this back to active.

Also removing Novice tag, until the false positives in the sniff reports are resolved. Such false positive sniffs require some judgement to be applied as to which fixes are required and what is inappropriate.

TravisCarden’s picture

Status: Active » Postponed

I appreciate your enthusiasm, @Lars Toomre, but this issue was postponed due to significant dependance on #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines, which hasn't been resolved yet.

sphism’s picture

Status: Postponed » Active

We have the go ahead with all these issues again, see #1518116: [meta] Make Core pass Coder Review for more details

TravisCarden’s picture

Assigned: TravisCarden » Unassigned
TravisCarden’s picture

Issue summary: View changes

Fixed a little typo.

sriharsha.uppuluri’s picture

Assigned: Unassigned » sriharsha.uppuluri
sriharsha.uppuluri’s picture

Status: Active » Needs review
FileSize
59.06 KB

Uploading patch.

Jalandhar’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch #49 needs reroll.

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Priority: Normal » Minor
Status: Needs work » Postponed
Issue tags: -Needs reroll

Thanks for all the work here so far. See #1518116-86: [meta] Make Core pass Coder Review. This issue is postponed until the meta issue is either closed or reopened.

xjm’s picture

Assigned: sriharsha.uppuluri » Unassigned
tatarbj’s picture

Version: 8.1.x-dev » 8.0.x-dev
Assigned: Unassigned » tatarbj
Status: Postponed » Needs review
FileSize
40.46 KB

I'm putting back the issue to 8.0.x version based on this issue and the started discussion there: https://www.drupal.org/node/1518116.
I'm also attach my patch which contains almost all of the fixed coding standards errors and warnings, except these:

phpcs --standard=Drupal -- core/modules/aggregator/

FILE: ...tor/src/Plugin/Field/FieldFormatter/AggregatorTitleFormatter.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 5 | WARNING | Line exceeds 80 characters; contains 84 characters
----------------------------------------------------------------------


FILE: ...gator/src/Plugin/Field/FieldFormatter/AggregatorXSSFormatter.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 5 | WARNING | Line exceeds 80 characters; contains 82 characters
----------------------------------------------------------------------


FILE: ...docs/drupal/core/modules/aggregator/src/Plugin/views/row/Rss.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 31 | ERROR | Class property $base_table should use lowerCamel naming
    |       | without underscores
 38 | ERROR | Class property $base_field should use lowerCamel naming
    |       | without underscores
----------------------------------------------------------------------


FILE: ...gator/tests/src/Unit/Plugin/AggregatorPluginSettingsBaseTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 5 | WARNING | Line exceeds 80 characters; contains 82 characters
----------------------------------------------------------------------


FILE: ...r/tests/src/Unit/Plugin/migrate/source/d6/AggregatorFeedTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 5 | WARNING | Line exceeds 80 characters; contains 86 characters
----------------------------------------------------------------------


FILE: ...r/tests/src/Unit/Plugin/migrate/source/d6/AggregatorItemTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 5 | WARNING | Line exceeds 80 characters; contains 86 characters
----------------------------------------------------------------------

Time: 3.54 secs; Memory: 16.25Mb

Those warning are unfixable because they contain the classnames which are too long ones. #2530920: Over-aggressive line length chokes on annotations
The errors in rss.php file is a deeper problem with historical cases that's why i was not able to rename those properties.

tatarbj’s picture

Status: Needs review » Fixed

I've created a new issue for following this up, i'm closing this, please check it: #2567365: Make Aggregator module pass Coder Review

TravisCarden’s picture

Status: Fixed » Needs review
pfrenssen’s picture

Status: Needs review » Closed (duplicate)

Closing in favor of #2571965: [meta] Fix PHP coding standards in core. In this issue the coding standards will be fixed on a sniff-per-sniff basis rather than a module-per-module basis.