Convert update_manager_install_form, update_manager_update_form, update_manager_update_ready_form to the new form interface.

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

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"

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

This blocks removal of MENU_LOCAL_ACTION.

This needed a big reroll, no interdiff was possible.

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

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

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.

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

Regards.

This needs a reroll for FormBase

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

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.

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

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.

Let's test it.

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

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

Updated patch:

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

Straight reroll.

Reroll.

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.

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

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

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

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

Changed status..

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

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

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

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

Fixes exception..

Rerolled.

Forgot to remove dead code.

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

Git mixup. Interdiff was right.

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

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.

Fair points. My interdiff matches your review.

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