Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Munavijayalakshmi created an issue. See original summary.

Munavijayalakshmi’s picture

Assigned: Munavijayalakshmi » Unassigned
Status: Active » Needs review
FileSize
11.99 KB
Wim Leers’s picture

Priority: Normal » Minor
Status: Needs review » Postponed

Please wait to commit this until more important issues are committed, which would otherwise need rerolls.

Wim Leers’s picture

Status: Postponed » Needs review
Issue tags: +Novice, +php-novice

This can now continue again. Retesting.

Status: Needs review » Needs work

The last submitted patch, 2: short_array_syntax-2869724-2.patch, failed testing.

Wim Leers’s picture

Issue tags: +Needs reroll
cjgratacos’s picture

Assigned: Unassigned » cjgratacos
cjgratacos’s picture

Assigned: cjgratacos » Unassigned
Status: Needs work » Needs review
FileSize
17.08 KB

Rerolling Patch

clemens.tolboom’s picture

I had some code style issues (PHP Storm formatted it for me). It looks OK to me now.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll
  1. +++ b/src/Controller/RestUIController.php
    @@ -99,41 +99,46 @@ class RestUIController implements ContainerInjectionInterface {
    -      $list[$status]['#attributes'] = array('class' => array('rest-ui-list-section', $status));
    ...
    +      $list[$status]['#attributes'] = [
    +        'class' => [
    +          'rest-ui-list-section',
    +          $status
    +        ]
    +      ];
    

    I don't see why we'd be reflowing the code too?

  2. +++ b/src/Form/RestUIForm.php
    @@ -356,17 +356,20 @@ class RestUIForm extends ConfigFormBase {
    +    foreach ($form_state->getValue([
    +      'wrapper',
    +      'methods'
    +    ]) as $method => $values) {
    

    Same here…

  3. +++ b/tests/src/Functional/RestUITest.php
    @@ -24,7 +24,10 @@ class RestUITest extends JavascriptTestBase {
    +    $permissions = [
    +      'administer site configuration',
    +      'administer rest resources'
    +    ];
    

    Here it seems sensible though!

  4. +++ b/tests/src/Functional/RestUITest.php
    @@ -46,12 +49,14 @@ class RestUITest extends JavascriptTestBase {
    +    $this->assertSession()
    +      ->fieldExists('wrapper[settings][authentication][cookie]');
    ...
    -    $page->findField('granularity')->selectOption(RestResourceConfigInterface::METHOD_GRANULARITY);
    +    $page->findField('granularity')
    +      ->selectOption(RestResourceConfigInterface::METHOD_GRANULARITY);
    
    @@ -66,7 +71,8 @@ class RestUITest extends JavascriptTestBase {
    -    $page->findField('granularity')->selectOption(RestResourceConfigInterface::RESOURCE_GRANULARITY);
    +    $page->findField('granularity')
    +      ->selectOption(RestResourceConfigInterface::RESOURCE_GRANULARITY);
    

    And here it doesn't…

leolandotan’s picture

Assigned: Unassigned » leolandotan

I'll try to work on the suggested changes by @Wim Leers.

leolandotan’s picture

Here I have applied the following:

  • Reverted the reflow of some lines of code.
  • Fixed a minor indentation issue.

Hope everything is in order.

leolandotan’s picture

Assigned: leolandotan » Unassigned

  • leolando.tan authored a8d8261 on 8.x-1.x
    Issue #2869724 by leolando.tan, clemens.tolboom, Munavijayalakshmi,...
clemens.tolboom’s picture

Status: Needs review » Fixed

Thanks 2 all!

clemens.tolboom’s picture

@cjgratacos congrats with your first Drupal patch in your first month as a member.

cjgratacos’s picture

@ clemens.tolboom Thanks, excited to be part of this awesome community

Status: Fixed » Closed (fixed)

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