Support from Acquia helps fund testing for Drupal Acquia logo

Comments

scottalan’s picture

Assigned: Unassigned » scottalan
-enzo-’s picture

Hey folks

I did some code cleanup included in the patch attached.

But there are some ERRORS reported by drupalcs, I don't know how to resolve

24 | ERROR | Missing function doc comment
24 | ERROR | No scope modifier specified for function "setUp"

-- see http://drupal.org/node/1545476 false positive "Missing function doc comment" error on Simple Test inherited methods

55 | ERROR | No scope modifier specified for function "testDefaultWidgetPropertiesAlter"

199 | ERROR | Do not use t() in hook_menu()

Also I can't excecute coder against drupal 8 modules. if you have any idea how to this, I will really appreciate your comments.

-enzo-’s picture

Status: Active » Needs work

I did the changes following the recommendations of drupalcs with this patch http://drupal.org/node/1518116#comment-6027004

Here the definition of my drupalcs alias

phpcs --standard=$HOME/drupalcs/Drupal/ruleset.xml --extensions=php,module,inc,install,test,profile,theme'

tim.plunkett’s picture

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -468,7 +472,8 @@ function field_ui_field_overview_form($form, &$form_state, $entity_type, $bundle
+      '#attributes' => array(
+        'class' => array('draggable', 'tabledrag-leaf', 'add-new')),

This should be split over multiple lines, or none at all

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -679,7 +705,12 @@ function _field_ui_field_overview_form_validate_add_new($form, &$form_state) {
+  if (array_filter(array(
+    $field['label'],
+    $field['field_name'],
+    $field['type'],
+    $field['widget_type'],
+    ))) {
     // Missing label.
     if (!$field['label']) {
       form_set_error('fields][_add_new_field][label', t('Add new field: you need to provide a label.'));
@@ -746,8 +777,12 @@ function _field_ui_field_overview_form_validate_add_existing($form, &$form_state

@@ -746,8 +777,12 @@ function _field_ui_field_overview_form_validate_add_existing($form, &$form_state
   if (isset($form_state['values']['fields']['_add_existing_field'])) {
     $field = $form_state['values']['fields']['_add_existing_field'];
 
-    // Validate if any information was provided in the 're-use existing field' row.
-    if (array_filter(array($field['label'], $field['field_name'], $field['widget_type']))) {
+    // Check if any information was provided in the 're-use existing field' row.
+    if (array_filter(array(
+      $field['label'],
+      $field['field_name'],
+      $field['widget_type'],
+      ))) {

I don't think this is in our coding standard.

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -1084,7 +1118,13 @@ function field_ui_display_overview_form($form, &$form_state, $entity_type, $bund
+              '#limit_validation_errors' => array(
+                array(

I'd join these two together

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -1084,7 +1118,13 @@ function field_ui_display_overview_form($form, &$form_state, $entity_type, $bund
+                ),
+              ),

I'd join these two together

+++ b/core/modules/field_ui/field_ui.moduleundefined
@@ -104,13 +104,20 @@ function field_ui_menu() {
+              $entity_type,
+              $bundle_arg),
...
+              $bundle_pos,
+              '%map'),

@@ -118,7 +125,11 @@ function field_ui_menu() {
+              $bundle_pos,
+              '%map'),

@@ -126,7 +137,11 @@ function field_ui_menu() {
+              $bundle_pos,
+              '%map'),

@@ -134,7 +149,11 @@ function field_ui_menu() {
+              $bundle_pos,
+              '%map'),

@@ -142,7 +161,11 @@ function field_ui_menu() {
+              $bundle_pos,
+              '%map'),

@@ -155,7 +178,11 @@ function field_ui_menu() {
+              $bundle_arg,
+              'default'),

@@ -173,14 +200,23 @@ function field_ui_menu() {
+                $bundle_arg,
+                $view_mode),
...
+                $access['access callback']),
+                $access['access arguments']),

Should end with a comma and move the ), to the next line

-enzo-’s picture

Hello tim

Thanks for your feedback , I did the changes I understand, but I dont' understand your comments

1)

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -1084,7 +1118,13 @@ function field_ui_display_overview_form($form, &$form_state, $entity_type, $bund
+              '#limit_validation_errors' => array(
+                array(
I'd join these two together

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -1084,7 +1118,13 @@ function field_ui_display_overview_form($form, &$form_state, $entity_type, $bund
+                ),
+              ),

2)

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -679,7 +705,12 @@ function _field_ui_field_overview_form_validate_add_new($form, &$form_state) {
+  if (array_filter(array(
+    $field['label'],
+    $field['field_name'],
+    $field['type'],
+    $field['widget_type'],
+    ))) {
     // Missing label.
     if (!$field['label']) {
       form_set_error('fields][_add_new_field][label', t('Add new field: you need to provide a label.'));
@@ -746,8 +777,12 @@ function _field_ui_field_overview_form_validate_add_existing($form, &$form_state

@@ -746,8 +777,12 @@ function _field_ui_field_overview_form_validate_add_existing($form, &$form_state
   if (isset($form_state['values']['fields']['_add_existing_field'])) {
     $field = $form_state['values']['fields']['_add_existing_field'];

-    // Validate if any information was provided in the 're-use existing field' row.
-    if (array_filter(array($field['label'], $field['field_name'], $field['widget_type']))) {
+    // Check if any information was provided in the 're-use existing field' row.
+    if (array_filter(array(
+      $field['label'],
+      $field['field_name'],
+      $field['widget_type'],
+      ))) {
I don't think this is in our coding standard.

If you can use this new patch and create an interdiff I will really appreciate to learn how to apply in other code clean I want to do.

Regards,

tim.plunkett’s picture

1)

I meant like

'#limit_validation_errors' => array(array(
)),

2) We don't usually break up function calls over multiple lines, it should just be left as is.

-enzo-’s picture

Assigned: scottalan » -enzo-
Status: Needs work » Needs review
FileSize
25.88 KB

Thanks tim

I did the last changes you recommend, I guess now this patch needs a community revision.

bleen’s picture

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageDisplayTest.phpundefined
@@ -101,11 +101,12 @@ class ManageDisplayTest extends FieldUiTestBase {
+    // mode (uses 'default'), and hidden in 'teaser' mode
+    // (uses custom settings).

This looks wrong. I think that only "settings)." should be on the new line

TravisCarden’s picture

Status: Needs review » Postponed

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

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

dcmul’s picture

Issue summary: View changes

am working on this right now.

swentel’s picture

Status: Active » Closed (duplicate)