Files: 
CommentFileSizeAuthor
#36 update-2010246-36.patch55.32 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] 58,024 pass(es), 336 fail(s), and 24 exception(s).
[ View ]
#35 update-2010246-30.patch56.57 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 59,246 pass(es).
[ View ]
#35 interdiff.txt7.82 KBpfrenssen
#28 update-2010246-28.patch56.73 KBgoogletorp
PASSED: [[SimpleTest]]: [MySQL] 59,066 pass(es).
[ View ]
#24 interdiff.txt726 bytesplopesc
#24 update-2010246-24.patch53.41 KBplopesc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch update-2010246-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 update-2010246-20.patch53.41 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 58,689 pass(es).
[ View ]
#20 interdiff.txt14.97 KBplopesc
#18 update-2010246-18.patch53.28 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 57,889 pass(es).
[ View ]
#17 update-2010246-17.patch53.2 KBplopesc
PASSED: [[SimpleTest]]: [MySQL] 57,939 pass(es).
[ View ]
#17 interdiff.txt2.91 KBplopesc
#13 update-2010246-13.patch52.27 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,527 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#10 update-convert-update-module-forms-to-the-new-form-interface-2010246-5.patch51.23 KBInternetDevels
FAILED: [[SimpleTest]]: [MySQL] 57,518 pass(es), 7 fail(s), and 2 exception(s).
[ View ]
#7 update-convert-update-module-forms-to-the-new-form-interface-2010246-4.patch52.2 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 55,946 pass(es).
[ View ]
#7 interdiff.txt8.51 KBInternetDevels
#5 interdiff.txt12.62 KBInternetDevels
#4 update-convert-update-module-forms-to-the-new-form-interface-2010246-3.patch51.19 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 56,469 pass(es).
[ View ]
#2 update-convert-update-module-forms-to-the-new-form-interface-2010246-2.patch50.48 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 56,059 pass(es).
[ View ]

Comments

Component:upload.module» update.module

Assigned:InternetDevels» Unassigned
Status:Active» Needs review
StatusFileSize
new50.48 KB
PASSED: [[SimpleTest]]: [MySQL] 56,059 pass(es).
[ View ]

Patch attached.

+++ b/core/modules/update/lib/Drupal/update/Access/UpdateManagerAccess.phpundefined
@@ -0,0 +1,38 @@
+ * Definition of \Drupal\update\Form\UpdateManagerAccess.

Should be "Contains \..."

+++ b/core/modules/update/lib/Drupal/update/Access/UpdateManagerAccess.phpundefined
@@ -0,0 +1,38 @@
+   * Implements AccessCheckInterface::applies().
...
+   * Implements AccessCheckInterface::access().

Should be {@inheritdoc}

+++ b/core/modules/update/lib/Drupal/update/Access/UpdateManagerAccess.phpundefined
@@ -0,0 +1,38 @@
+    if (settings()->get('allow_authorize_operations', TRUE)) {

Let's inject the settings into this class.

+++ b/core/modules/update/lib/Drupal/update/Access/UpdateManagerAccess.phpundefined
@@ -0,0 +1,38 @@
+  }

Empty line needed here.

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateManagerInstall.phpundefined
@@ -0,0 +1,215 @@
+   * The module hendler.

hendler ^^. Let's call the variable moduleHandler and document it, to be a little more consistent.

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateManagerInstall.phpundefined
@@ -0,0 +1,215 @@
+   * Implements \Drupal\Core\Form\FormInterface::getFormID().
...
+   * Implements \Drupal\Core\Form\FormInterface::buildForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::validateForm().
...
+   * Implements \Drupal\Core\Form\FormInterface::submitForm().

This could be just {@inheritdoc}

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateManagerUpdate.phpundefined
@@ -0,0 +1,305 @@
+   * @param \Drupal\Core\Extension\CachedModuleHandler $handler
+   * The database connection.

Let's be consistent with the other file :) No it's not a db connection.

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateManagerUpdate.phpundefined
@@ -0,0 +1,305 @@
+    $form['#attached']['css'][] = drupal_get_path('module', 'update') . '/update.css';

Let's open a follow up to move this to a library.

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateManagerUpdate.phpundefined
@@ -0,0 +1,305 @@
+          $project_name = check_plain($project['title']);
...
+        $project_name = check_plain($project['info']['name']);
...
+        $project_name = check_plain($name);

check_plain() can be replaced by String::checkPlain()

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateManagerUpdate.phpundefined
@@ -0,0 +1,305 @@
+      $form['#validate'][] = 'update_manager_update_form_validate';

Isn't that the exact code of this validateForm method? I guess we can drop it

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateManagerUpdate.phpundefined
@@ -0,0 +1,305 @@
+      $table = array(
+        '#theme' => 'table',
+        '#header' => $headers,
+        '#rows' => $projects['manual'],
+      );
+      $form['manual_updates'] = array(
+        '#type' => 'markup',
+        '#markup' => drupal_render($table),
+        '#prefix' => $prefix,
+        '#weight' => 120,

Just wondering why we can't use just #theme table + a prefix/weight set on that render element.

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateReady.phpundefined
@@ -0,0 +1,152 @@
+  /**
+   * Implements \Drupal\Core\Form\FormInterface::validateForm().
+   */
+  public function validateForm(array &$form, array &$form_state) {
+    parent::validateForm($form, $form_state);
+  }

If you just call the parent method you can just drop the function completely.

+++ b/core/modules/update/update.moduleundefined
@@ -223,12 +215,9 @@ function update_menu() {
   $items['admin/update/ready'] = array(
     'title' => 'Ready to update',
-    'page callback' => 'drupal_get_form',
-    'page arguments' => array('update_manager_update_ready_form'),
-    'access callback' => 'update_manager_access',
-    'access arguments' => array(),
+    'route_name' => 'update_ready',
     'type' => MENU_CALLBACK,
-    'file' => 'update.manager.inc',

That's a menu callback.

+++ b/core/modules/update/update.routing.ymlundefined
@@ -11,3 +11,59 @@ update_status:
+    ¶
...
+    _form: '\Drupal\update\Form\UpdateManagerInstall' ¶
...
+    ¶
...
+    ¶

trailing whitespace

StatusFileSize
new51.19 KB
PASSED: [[SimpleTest]]: [MySQL] 56,469 pass(es).
[ View ]

Thanks for your review. All issues fixed.

StatusFileSize
new12.62 KB

Forgot to put interdiff in the previous comment.

Thank you!
Just some small points left.

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateManagerUpdate.phpundefined
@@ -0,0 +1,301 @@
+  protected $handler;
...
+   * @param \Drupal\Core\Extension\CachedModuleHandler $handler

Use the moduleHandlerInterface in the method and document the $handler variable. It would be cool to be consistent with all other parts and name it $this->moduleHandler

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateReady.phpundefined
@@ -0,0 +1,147 @@
+  protected $handler;
+  protected $configFactory;
...
+  /**
+   * Constructs a new UpdateReady object.
+   *
+   * @param \Drupal\Core\Extension\ModuleHandlerInterface $handler
+   * The module handler.
+   * ¶
+   * @param \Drupal\Core\Config\ConfigFactory $configFactory
+   * The Config factory.
+   *
+   * @param \Drupal\Core\Config\Context\ContextInterface $context
+   * The config Interface.

Sorry. This has wrong indentation, a leftover whitespace and missing documentation.

+++ b/core/modules/update/update.manager.incundefined
@@ -482,63 +89,23 @@ function update_manager_update_ready_form_submit($form, &$form_state) {
+  $last = state()->get('update.last_check') ?: 0;

I guess Drupal::state() should be used instead.

+++ b/core/modules/update/update.moduleundefined
@@ -150,6 +150,24 @@ function update_init() {
+ * Implements hook_library_info().
...
+function update_library_info() {

Nice!

+++ b/core/modules/update/lib/Drupal/update/Access/UpdateManagerAccess.phpundefined
@@ -19,17 +20,34 @@
+   *   The settings array.

I would say that this is more then just the settings array.

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateManagerInstall.phpundefined
@@ -23,14 +23,14 @@ class UpdateManagerInstall implements FormInterface, ControllerInterface {
    * @param \Drupal\Core\Extension\ModuleHandlerInterface $handler
-   * The module hendler.
+   *  The Module Handler.

There should be in total three spaces before the "The"

StatusFileSize
new8.51 KB
new52.2 KB
PASSED: [[SimpleTest]]: [MySQL] 55,946 pass(es).
[ View ]

Thanks for your new review. All issues fixed.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateManagerInstall.phpundefined
@@ -0,0 +1,215 @@
+    $archive_errors = update_manager_archive_verify($project, $local_cache, $directory);

This could be replaced with the injected module handler. $archive_errors = $this->moduleHandler->invokeAll('verify_update_archive', array($project, $local_cache, $directory))

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateManagerInstall.phpundefined
@@ -0,0 +1,215 @@
+    // Make sure the Updater registry is loaded.
+    drupal_get_updaters();

I can't see why this is required...

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateReady.phpundefined
@@ -0,0 +1,148 @@
+      // Make sure the Updater registry is loaded.
+      drupal_get_updaters();

Same again... Is this needed?

Status:Needs work» Needs review
StatusFileSize
new51.23 KB
FAILED: [[SimpleTest]]: [MySQL] 57,518 pass(es), 7 fail(s), and 2 exception(s).
[ View ]

Thanks for your new review. All issues are fixed.
The interdiff.txt was not added because old patch was not applied to new version of the Update module. A new patch was generated.

Status:Needs review» Needs work

Issue tags:+MENU_LOCAL_ACTION

This blocks removal of MENU_LOCAL_ACTION.

Status:Needs work» Needs review
StatusFileSize
new52.27 KB
FAILED: [[SimpleTest]]: [MySQL] 57,527 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This needed a big reroll, no interdiff was possible.

#13: update-2010246-13.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+FormInterface, +WSCCI-conversion, +CodeSprintUA, +MENU_LOCAL_ACTION

The last submitted patch, update-2010246-13.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.91 KB
new53.2 KB
PASSED: [[SimpleTest]]: [MySQL] 57,939 pass(es).
[ View ]

Hello

Re-rolling, patch failed because the form theme function was removed from update.manager.inc and the Update page form was not displayed.

This patch removes this theme function and adds this functionality to the UpdateManagerUpdate class.

It should be green now.

Regards.

StatusFileSize
new53.28 KB
PASSED: [[SimpleTest]]: [MySQL] 57,889 pass(es).
[ View ]

Re-rolling this patch, once #2048933: Replace theme() with drupal_render() in update module has been committed :)

Regards.

Status:Needs review» Needs work

This needs a reroll for FormBase

Status:Needs work» Needs review
StatusFileSize
new14.97 KB
new53.41 KB
PASSED: [[SimpleTest]]: [MySQL] 58,689 pass(es).
[ View ]

Re-rolling.

Regards.

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

Some minor changes. After these are complete, I'm fine RTBC'ing this if tests pass, but we really need to get this into an update service (which should be opened as a follow-up). Note that that service would be used by more than just this, anything having to do with batch operations would use it as well.

  1. +++ b/core/modules/update/lib/Drupal/update/Form/UpdateManagerInstall.php
    @@ -0,0 +1,223 @@
    +      $this->moduleHandler->loadInclude('update', 'inc', 'update.authorize');

    lets get these methods in update.module as well.

  2. +++ b/core/modules/update/lib/Drupal/update/Form/UpdateManagerUpdate.php
    @@ -0,0 +1,326 @@
    +    $this->moduleHandler->loadInclude('update', 'inc', 'update.manager');

    can we move these functions to update.module?

  3. +++ b/core/modules/update/lib/Drupal/update/Form/UpdateManagerUpdate.php
    @@ -0,0 +1,326 @@
    +      'file' => drupal_get_path('module', 'update') . '/update.manager.inc',

    if we move methods to update.module, we can get rid of this line.

  4. +++ b/core/modules/update/lib/Drupal/update/Form/UpdateReady.php
    @@ -0,0 +1,146 @@
    +    drupal_set_title($this->t('Ready to update'));

    $form['#title']

  5. +++ b/core/modules/update/lib/Drupal/update/Form/UpdateReady.php
    @@ -0,0 +1,146 @@
    +        $this->moduleHandler->loadInclude('update', 'inc', 'update.authorize');

    here as well

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new53.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch update-2010246-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new726 bytes

After talk with @disasm and @timplunkett on IRC, decided not to move code to update.module.

This patch just get rid of drupal_set_title().

Regards.

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateManagerInstall.php
@@ -0,0 +1,223 @@
+    elseif ($_FILES['files']['name']['project_upload']) {

we can use $request here.

Status:Needs review» Needs work
Issue tags:+FormInterface, +WSCCI-conversion, +CodeSprintUA, +MENU_LOCAL_ACTION

The last submitted patch, update-2010246-24.patch, failed testing.

StatusFileSize
new56.73 KB
PASSED: [[SimpleTest]]: [MySQL] 59,066 pass(es).
[ View ]

Rerolled the patch since #2089635: Convert non-test non-form page callbacks to routes and controllers broke this patch.

I've used the . notation in the routing files which was introduced in the patch from above.

I've also add the removal of some of the code introduces in the above patch which no longer is needed.

Status:Needs work» Needs review

Let's test it.

Status:Needs review» Needs work
Issue tags:-FormInterface, -WSCCI-conversion, -CodeSprintUA, -MENU_LOCAL_ACTION

The last submitted patch, update-2010246-28.patch, failed testing.

Status:Needs work» Needs review

#28: update-2010246-28.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, update-2010246-28.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+FormInterface, +WSCCI-conversion, +CodeSprintUA, +MENU_LOCAL_ACTION

#28: update-2010246-28.patch queued for re-testing.

Re testing this as it was two failures with displayblocktest that failed, and checked locally and they seem to have just been random failures.

+++ b/core/modules/update/lib/Drupal/update/Access/UpdateManagerAccess.phpundefined
@@ -0,0 +1,51 @@
+  /**
+   * The object which works with settings is in settings.php.
+   *
+   * @var \Drupal\Component\Utility\Settings
+   */
+  protected $settings;
+

It is not clear to me what this documentation means.

+++ b/core/modules/update/lib/Drupal/update/Access/UpdateManagerAccess.phpundefined
@@ -0,0 +1,51 @@
+  /**
+   * Constructs UpdateManagerAccess object.
+   *
+   * @param \Drupal\Component\Utility\Settings $settings

Constructs an UpdateManagerAccess object.

Issue tags:-CodeSprintUA
StatusFileSize
new7.82 KB
new56.57 KB
PASSED: [[SimpleTest]]: [MySQL] 59,246 pass(es).
[ View ]

Updated patch:

  • Addressed my remarks from #34.
  • Converted hook_local_actions() to yaml.
  • Some minor cleanups.

Issue summary:View changes
StatusFileSize
new55.32 KB
FAILED: [[SimpleTest]]: [MySQL] 58,024 pass(es), 336 fail(s), and 24 exception(s).
[ View ]

Straight reroll.

Status:Needs review» Needs work

The last submitted patch, 36: update-2010246-36.patch, failed testing.