CommentFileSizeAuthor
#69 interdiff.txt3.08 KBtim.plunkett
#69 update-2010246-69.patch51.8 KBtim.plunkett
#65 update-2010246-65.patch51.86 KBtim.plunkett
#64 interdiff.txt543 bytestim.plunkett
#64 update-2010246-64.patch99.38 KBtim.plunkett
#61 interdiff.txt2.41 KBtim.plunkett
#61 update-2010246-61.patch51.33 KBtim.plunkett
#60 update-2010246-60.patch48.92 KBtim.plunkett
#58 update-2010246-58.patch58.61 KBtkuldeep17
#56 update-2010246-55.patch58.57 KBtkuldeep17
#53 update-2010246-53.patch58.57 KBtkuldeep17
#48 update-2010246-48.patch68.59 KBtkuldeep17
#45 interdiff-40-45.txt1.87 KBtkuldeep17
#45 update-2010246-45.patch64.32 KBtkuldeep17
#42 update-2010246-40.patch62.1 KBtkuldeep17
#40 interdiff-38-40.txt5.33 KBtkuldeep17
#40 update-2010246-38.patch55.31 KBtkuldeep17
#38 update-2010246-38.patch55.32 KBtim.plunkett
#36 update-2010246-36.patch55.32 KBpfrenssen
#35 update-2010246-30.patch56.57 KBpfrenssen
#35 interdiff.txt7.82 KBpfrenssen
#28 update-2010246-28.patch56.73 KBgoogletorp
#24 interdiff.txt726 bytesplopesc
#24 update-2010246-24.patch53.41 KBplopesc
#20 update-2010246-20.patch53.41 KBplopesc
#20 interdiff.txt14.97 KBplopesc
#18 update-2010246-18.patch53.28 KBplopesc
#17 update-2010246-17.patch53.2 KBplopesc
#17 interdiff.txt2.91 KBplopesc
#13 update-2010246-13.patch52.27 KBtim.plunkett
#10 update-convert-update-module-forms-to-the-new-form-interface-2010246-5.patch51.23 KBInternetDevels
#7 update-convert-update-module-forms-to-the-new-form-interface-2010246-4.patch52.2 KBInternetDevels
#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
#2 update-convert-update-module-forms-to-the-new-form-interface-2010246-2.patch50.48 KBInternetDevels
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

InternetDevels’s picture

Component: upload.module » update.module
InternetDevels’s picture

Assigned: InternetDevels » Unassigned
Status: Active » Needs review
FileSize
50.48 KB

Patch attached.

dawehner’s picture

+++ 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

InternetDevels’s picture

Thanks for your review. All issues fixed.

InternetDevels’s picture

FileSize
12.62 KB

Forgot to put interdiff in the previous comment.

dawehner’s picture

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"

InternetDevels’s picture

Thanks for your new review. All issues fixed.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

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?

InternetDevels’s picture

Status: Needs work » Needs review
FileSize
51.23 KB

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
tim.plunkett’s picture

Issue tags: +MENU_LOCAL_ACTION

This blocks removal of MENU_LOCAL_ACTION.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
52.27 KB

This needed a big reroll, no interdiff was possible.

dawehner’s picture

tim.plunkett’s picture

#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.

plopesc’s picture

Status: Needs work » Needs review
FileSize
2.91 KB
53.2 KB

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.

plopesc’s picture

FileSize
53.28 KB

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

Regards.

tim.plunkett’s picture

Status: Needs review » Needs work

This needs a reroll for FormBase

plopesc’s picture

Status: Needs work » Needs review
FileSize
14.97 KB
53.41 KB

Re-rolling.

Regards.

xjm’s picture

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.

disasm’s picture

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

disasm’s picture

Status: Needs review » Needs work
plopesc’s picture

Status: Needs work » Needs review
FileSize
53.41 KB
726 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.

jibran’s picture

+++ 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.

googletorp’s picture

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

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

googletorp’s picture

FileSize
56.73 KB

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.

pfrenssen’s picture

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.

googletorp’s picture

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.

ACF’s picture

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.

pfrenssen’s picture

+++ 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.

pfrenssen’s picture

Issue tags: -CodeSprintUA
FileSize
7.82 KB
56.57 KB

Updated patch:

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

Issue summary: View changes
FileSize
55.32 KB

Straight reroll.

Status: Needs review » Needs work

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

Status: Needs review » Needs work

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

tkuldeep17’s picture

Status: Needs work » Needs review
FileSize
55.31 KB
5.33 KB

When I applied patch in my local setup, I faced some errors. The errors are as following.

  • The method _update_manager_check_backends() is not defined
  • We are injecting service $container->get('state'). This service returns Drupal\Core\State\StateInterface type object instead of Drupal\Core\KeyValueStore\KeyValueStoreInterface.

For fixing it steps taken by me

  • Restored function _update_manager_check_backends() in manager.update.inc
  • Changed Drupal\Core\State\StateInterface from Drupal\Core\KeyValueStore\KeyValueStoreInterface.

Status: Needs review » Needs work

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

tkuldeep17’s picture

FileSize
62.1 KB

Sorry same patch of tim.plunkett submitted.. Now submitting new patch.. :-)

tkuldeep17’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

tkuldeep17’s picture

Status: Needs work » Needs review
FileSize
64.32 KB
1.87 KB

Class Drupal\system\SystemConfigFormBase does not exist. So I have changed to Drupal\Core\Form\FormBase class for creating form in UpdateReady.php file.

Status: Needs review » Needs work

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

tim.plunkett’s picture

That should be \Drupal\Core\Form\ConfigFormBase, and use the $this->config() method instead of $this->configFactory->get().

tkuldeep17’s picture

FileSize
68.59 KB

@tim.plunkett
Thank you for reviewing patch. Fixed your point..

tkuldeep17’s picture

Status: Needs work » Needs review

Changed status..

Status: Needs review » Needs work

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

tkuldeep17’s picture

@tim.plunkett
By seeing cause of failing my patch, it is failing in xmlrpc test. Why it is so.. need help...

tim.plunkett’s picture

+++ b/core/modules/update/lib/Drupal/update/Form/UpdateReady.php
@@ -0,0 +1,146 @@
+class UpdateReady extends SystemConfigFormBase {
...
+      $container->get('config.context.free')

There is no SystemConfigFormBase. It is \Drupal\Core\Form\ConfigFormBase, as I said before.
And it doesn't need the config context.

tkuldeep17’s picture

FileSize
58.57 KB

Sorry, I created path with old code. Now uploading updated patch.

tkuldeep17’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

tkuldeep17’s picture

Status: Needs work » Needs review
FileSize
58.57 KB

Very Sorry for repeatedly uploading wrong patch.. Uploading correct patch..

Status: Needs review » Needs work

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

tkuldeep17’s picture

Status: Needs work » Needs review
FileSize
58.61 KB

Fixes exception..

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
48.92 KB

Rerolled.

tim.plunkett’s picture

FileSize
51.33 KB
2.41 KB

Forgot to remove dead code.

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

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
99.38 KB
543 bytes

We removed the theme function, we have to remove the definition.

tim.plunkett’s picture

FileSize
51.86 KB

Git mixup. Interdiff was right.

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

jibran’s picture

Status: Needs review » Reviewed & tested by the community

It's green. I haven't found anything coding wise so RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. diff --git a/core/modules/update/src/Form/UpdateManagerInstall.php b/core/modules/update/src/Form/UpdateManagerInstall.php
    index b65cc20..e44cd0d 100644
    --- a/core/modules/update/src/Form/UpdateManagerInstall.php
    +++ b/core/modules/update/src/Form/UpdateManagerInstall.php
    @@ -104,7 +104,7 @@ public function buildForm(array $form, array &$form_state) {
       public function validateForm(array &$form, array &$form_state) {
         $uploaded_file = $this->getRequest()->files->get('files[project_upload]', NULL, TRUE);
         if (!($form_state['values']['project_url'] XOR !empty($uploaded_file))) {
    -      form_set_error('project_url', $form_state, $this->t('You must either provide a URL or upload an archive file to install.'));
    +      $this->setFormError('project_url', $form_state, $this->t('You must either provide a URL or upload an archive file to install.'));
         }
       }
    

    Deprecated function use.

  2. @@ -112,10 +112,8 @@ public function validateForm(array &$form, array &$form_state) {
        * {@inheritdoc}
        */
       public function submitForm(array &$form, array &$form_state) {
    -    $field = NULL;
         $local_cache = NULL;
         if ($form_state['values']['project_url']) {
    -      $field = 'project_url';
           $local_cache = update_manager_file_get($form_state['values']['project_url']);
           if (!$local_cache) {
             drupal_set_message($this->t('Unable to retrieve Drupal project from %url.', array('%url' => $form_state['values']['project_url'])), 'error');
    @@ -124,8 +122,7 @@ public function submitForm(array &$form, array &$form_state) {
         }
         elseif ($_FILES['files']['name']['project_upload']) {
           $validators = array('file_validate_extensions' => array(archiver_get_extensions()));
    -      $field = 'project_upload';
    -      if (!($finfo = file_save_upload($field, $validators, NULL, 0, FILE_EXISTS_REPLACE))) {
    +      if (!($finfo = file_save_upload('project_upload', $validators, NULL, 0, FILE_EXISTS_REPLACE))) {
             // Failed to upload the file. file_save_upload() calls
             // drupal_set_message() on failure.
             return;
    

    Unnecessary variables.

  3. diff --git a/core/modules/update/src/Form/UpdateManagerUpdate.php b/core/modules/update/src/Form/UpdateManagerUpdate.php
    index d63d3cc..fd2cfb2 100644
    --- a/core/modules/update/src/Form/UpdateManagerUpdate.php
    +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
    @@ -293,7 +293,7 @@ public function validateForm(array &$form, array &$form_state) {
           $disabled = array_filter($form_state['values']['disabled_projects']);
         }
         if (empty($enabled) && empty($disabled)) {
    -      form_set_error('projects', $form_state, $this->t('You must select at least one project to update.'));
    +      $this->setFormError('projects', $form_state, $this->t('You must select at least one project to update.'));
         }
       }
    

    Deprecated function use.

  4. diff --git a/core/modules/update/update.manager.inc b/core/modules/update/update.manager.inc
    index 66d9647..1faf7fd 100644
    --- a/core/modules/update/update.manager.inc
    +++ b/core/modules/update/update.manager.inc
    @@ -36,8 +36,6 @@
      * root.
      */
     
    -use Drupal\Core\Updater\Updater;
    -use Drupal\Core\FileTransfer\Local;
     use Symfony\Component\HttpFoundation\RedirectResponse;
     
     /**
    

    Unused uses.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
51.8 KB
3.08 KB

Fair points. My interdiff matches your review.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 8f94da5 and pushed to 8.x. Thanks!

  • alexpott committed 8f94da5 on
    Issue #2010246 by tim.plunkett, tkuldeep17, plopesc, InternetDevels,...

Status: Fixed » Closed (fixed)

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