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

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Update the issue summary Yes Instructions
Add automated tests Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nick_schuch’s picture

Status: Active » Needs review
FileSize
33.29 KB

First attempt.

NROTC_Webmaster’s picture

Status: Needs review » Postponed

Unfortunately, you shouldn't make any changes to the docblocks because #1313980: Clean up API docs for node module is still in progress. At a minimum this will need a reroll after the other issue is fixed. I would suggest picking another issue to work on until then.

Lars Toomre’s picture

Status: Postponed » Needs work

The issue #1313980: Clean up API docs for node module is mostly complete (except for a review of the Test classes).

As a result, it would be great if someone could post here at least the results of running this module through the D7 Coder with the appropriate settings. At present, I am unable to do so and would really like to see how close this key core module is to passing a full Coder review.

I think that we can then roll a revised patch addresses many of the flagged items. Thanks in advance.

TravisCarden’s picture

Status: Needs work » Postponed

Postponing till feature freeze. If you want to help in the meantime, please work on the blockers on the meta issue. Thanks!

xjm’s picture

Component: node.module » node system
Issue summary: View changes

(Merging "node system" and "node.module" components for 8.x; disregard.)

TravisCarden’s picture

Status: Postponed » Active
blueminds’s picture

Status: Active » Needs review
FileSize
3.82 KB

please see the patch.

There are still 5 minor warnings that come from test assertions like
$this->assertRaw('>' . $newterm1 . '<', 'First new term displayed.');

minneapolisdan’s picture

Issue summary: View changes
blueminds’s picture

Status: Needs review » Needs work

The last submitted patch, 9: make-node-module-pass-coder-review-1533254-9.patch, failed testing.

henk’s picture

Patch is rerolled, but I found also some nasty code in node.module line 107, this should be placed in template in my opinion:

case 'help.page.node':
      $output = '';
      $output .= '<h3>' . t('About') . '</h3>';
      $output .= '<p>' . t('The Node module manages the creation, editing, deletion, settings, and display of the main site content. Content items managed by the Node module are typically displayed as pages on your site, and include a title, some meta-data (author, creation time, content type, etc.), and optional fields containing text or other data (fields are managed by the <a href="!field">Field module</a>). For more information, see <a href="!node">the online documentation for the Node module</a>.', array('!node' => 'https://drupal.org/documentation/modules/node', '!field' => \Drupal::url('help.page', array('name' => 'field')))) . '</p>';
      $output .= '<h3>' . t('Uses') . '</h3>';
      $output .= '<dl>';
      $output .= '<dt>' . t('Creating content') . '</dt>';
      $output .= '<dd>' . t('When new content is created, the Node module records basic information about the content, including the author, date of creation, and the <a href="!content-type">Content type</a>. It also manages the <em>publishing options</em>, which define whether or not the content is published, promoted to the front page of the site, and/or sticky at the top of content lists. Default settings can be configured for each <a href="!content-type">type of content</a> on your site.', array('!content-type' => \Drupal::url('node.overview_types'))) . '</dd>';
      $output .= '<dt>' . t('Creating custom content types') . '</dt>';
      $output .= '<dd>' . t('The Node module gives users with the <em>Administer content types</em> permission the ability to <a href="!content-new">create new content types</a> in addition to the default ones already configured. Creating custom content types allows you the flexibility to add <a href="!field">fields</a> and configure default settings that suit the differing needs of various site content.', array('!content-new' => \Drupal::url('node.type_add'), '!field' => \Drupal::url('help.page', array('name' => 'field')))) . '</dd>';
      $output .= '<dt>' . t('Administering content') . '</dt>';
      $output .= '<dd>' . t('The <a href="!content">Content administration page</a> allows you to review and bulk manage your site content.', array('!content' => \Drupal::url('system.admin_content'))) . '</dd>';
      $output .= '<dt>' . t('Creating revisions') . '</dt>';
      $output .= '<dd>' . t('The Node module also enables you to create multiple versions of any content, and revert to older versions using the <em>Revision information</em> settings.') . '</dd>';
      $output .= '<dt>' . t('User permissions') . '</dt>';
      $output .= '<dd>' . t('The Node module makes a number of permissions available for each content type, which can be set by role on the <a href="!permissions">permissions page</a>.', array('!permissions' => \Drupal::url('user.admin_permissions', array(), array('fragment' => 'module-node')))) . '</dd>';
      $output .= '</dl>';
      return $output;

henk’s picture

Status: Needs work » Needs review
henk’s picture

status to needs review

omessaoudi’s picture

I tried to apply the patch that does not pass. I'll try to rerol

omessaoudi’s picture

This is the patch rerolled

xjm’s picture

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

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.

tatarbj’s picture

Status: Postponed » 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.