Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

CommentFileSizeAuthor
#76 interdiff.txt15.06 KBkim.pepper
#76 1987888-update-manual-status-76.patch49.28 KBkim.pepper
#74 interdiff.txt11.36 KBkim.pepper
#74 1987888-update-manual-status-74.patch50.18 KBkim.pepper
#70 1987888-update-manual-status-70.patch43.79 KBkim.pepper
#63 interdiff.txt13.61 KBkim.pepper
#63 1987888-update-manual-status-63.patch43.75 KBkim.pepper
#59 1987888-update-manual-status-59.patch41.74 KBzengenuity
#57 1987888-update-manual-status-57.patch21.02 KBzengenuity
#55 drupal8.update-module.1987888-55.patch62.28 KBdisasm
#53 1987888-update-manual-status-to-controller-53.patch2.14 KBJB13
#41 update-1987888-41.patch2.65 KBdawehner
#41 interdiff.txt1.09 KBdawehner
#39 drupal8.update-module.1987888-39.patch2.53 KBdisasm
#39 interdiff.txt3.06 KBdisasm
#37 1987888-update-manual-status-37.patch3.55 KBzengenuity
#32 1987888-update-manual-status-32.patch134.32 KBzengenuity
#29 1987888-update-manual-status-29.patch65.36 KBvijaycs85
#26 1987888-update-manual-status-26.patch107.79 KBzengenuity
#23 1987888-update-manual-status-23.patch126.92 KBzengenuity
#21 1987888-update-manual-status-21.patch126.93 KBzengenuity
#21 interdiff-19-21.txt24.24 KBzengenuity
#19 1987888-update-manual-status-19.patch126.3 KBzengenuity
#19 interdiff-17-19.txt2.2 KBzengenuity
#19 interdiff-15-19.txt82.5 KBzengenuity
#17 1987888-update-manual-status-17.patch124.71 KBzengenuity
#17 interdiff.txt81.25 KBzengenuity
#15 1987888-update-manual-status-15.patch67.3 KBzengenuity
#15 interdiff.txt1.5 KBzengenuity
#11 1987888-update-manual-status-11.patch67.29 KBvijaycs85
#11 1987888-diff-9-11.txt4.59 KBvijaycs85
#9 update_manual_status-1987888-9.patch67.38 KBzengenuity
#7 update_manual_status-1987888-7.patch48.08 KBzengenuity
#5 update_manual_status-1987888-5.patch43.29 KBzengenuity
#3 update_manual_status-1987888-2.patch43.44 KBzengenuity
#2 update_manual_status-1987888-2.patch43.44 KBzengenuity
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zengenuity’s picture

Assigned: Unassigned » zengenuity

Taking a swing at this.

zengenuity’s picture

The attached patch reworks update_manual_status() to use the new style controller. The code from update.fetch.inc is now found in UpdateFetchController.php and UpdateFetchManager.php. In addition, I started the process of converting update.compare.inc to UpdateCompareManager.php so that I could use dependency injection for the one related function from update.compare.inc. I left the rest update.compare.inc in place, since I wasn't sure if I'm doing this 100% correctly. (this is my first WSCCI controller conversion)

In the process of doing this conversion, I ran into a question about how to pass methods from the new controllers to batch_set(). It appears that using something like \Drupal\update\Controller\UpdateFetchController::updateFetchDataBatch works in the operations key, but not in the finished key. So, I had to move the finished function into update.module. (since I've deleted update.fetch.inc) This is probably not the best solution here, but I didn't want to start messing around with batch_finished(). (There is a function_exists() call that doesn't work with the new style path and prevents the finished callback from firing if it's in the new style.)

This is my first WSCCI controller conversion, so feedback is appreciated. Not 100% sure I'm doing this right, but it does seem to work.

zengenuity’s picture

Status: Active » Needs review
FileSize
43.44 KB

Changing status to Needs Review

Status: Needs review » Needs work

The last submitted patch, update_manual_status-1987888-2.patch, failed testing.

zengenuity’s picture

Status: Needs work » Needs review
FileSize
43.29 KB

Bad formatting of patch. Let's try this one.

Status: Needs review » Needs work

The last submitted patch, update_manual_status-1987888-5.patch, failed testing.

zengenuity’s picture

Status: Needs work » Needs review
FileSize
48.08 KB

Some issues with the tests themselves. Running this though testbot one more time before heading to the code sprint.

Status: Needs review » Needs work

The last submitted patch, update_manual_status-1987888-7.patch, failed testing.

zengenuity’s picture

Status: Needs work » Needs review
FileSize
67.38 KB

Trying this again.

Status: Needs review » Needs work

The last submitted patch, update_manual_status-1987888-9.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
4.59 KB
67.29 KB

Re-roll with minor code and interface fixes...

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, 1987888-update-manual-status-11.patch, failed testing.

zengenuity’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, 1987888-update-manual-status-11.patch, failed testing.

zengenuity’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
67.3 KB

Fixed the tests that failed last time. Let's see how tests go this time.

dawehner’s picture

Good work so far!

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateFetchController.phpundefined
@@ -0,0 +1,97 @@
+ *
+ * Code required only when fetching information about available updates.

I guess this comment can be dropped because the information on there is not really helpful

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateFetchController.phpundefined
@@ -0,0 +1,97 @@
+   * Page callback: Checks for updates and displays the update status report.

Please remove the 'Page callback' in front, that's an old c&p of the documentation.

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateFetchController.phpundefined
@@ -0,0 +1,97 @@
+    \Drupal::service('update.fetch')->refresh();

You should inject the update.fetch service.

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateFetchController.phpundefined
@@ -0,0 +1,97 @@
+        array('\Drupal\update\Controller\UpdateFetchController::fetchDataBatch', array()),
...
+  public static function fetchDataBatch(&$context) {

I'm wondering whether batch supports callables (so an instance of the object) as well.

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateFetchController.phpundefined
@@ -0,0 +1,97 @@
+    $queue = \Drupal::queue('update_fetch_tasks');

Because then you could inject the queue service in there as well.

+++ b/core/modules/update/lib/Drupal/update/Tests/UpdateCoreTest.phpundefined
@@ -195,6 +196,7 @@ function testServiceUnavailable() {
   function testFetchTasks() {
+    $service = new UpdateFetchManager();

This is really cool and allows maybe one day a proper unit test.

+++ b/core/modules/update/lib/Drupal/update/Tests/UpdateCoreUnitTest.phpundefined
@@ -31,13 +33,13 @@ public static function getInfo() {
   function setUp() {
     parent::setUp();
-    module_load_include('inc', 'update', 'update.fetch');

Wouldn't it be possible to drop the setUp method in total here?

+++ b/core/modules/update/lib/Drupal/update/Tests/UpdateCoreUnitTest.phpundefined
@@ -46,14 +48,16 @@ function testUpdateBuildFetchUrl() {
+    //$url = \Drupal::service('update.fetch')->buildFetchUrl($project, $site_key);
...
+    //$url = \Drupal::service('update.fetch')->buildFetchUrl($project, $site_key);

@@ -61,7 +65,8 @@ function testUpdateBuildFetchUrl() {
+    //$url = \Drupal::service('update.fetch')->buildFetchUrl($project, $site_key);

@@ -70,7 +75,8 @@ function testUpdateBuildFetchUrl() {
+    //$url = \Drupal::service('update.fetch')->buildFetchUrl($project, $site_key);

So let's drop these lines

+++ b/core/modules/update/lib/Drupal/update/UpdateCompareManager.phpundefined
@@ -0,0 +1,354 @@
+ *
+ * Code required only when comparing available updates to existing data.

See first comment.

+++ b/core/modules/update/lib/Drupal/update/UpdateCompareManager.phpundefined
@@ -0,0 +1,354 @@
+   * @return

Should be @return array. Please have a look at all the other docs as they need some small polishing as well.

+++ b/core/modules/update/lib/Drupal/update/UpdateCompareManager.phpundefined
@@ -0,0 +1,354 @@
+  function getProjects() {

I guess this is a public method?

+++ b/core/modules/update/lib/Drupal/update/UpdateCompareManager.phpundefined
@@ -0,0 +1,354 @@
+    $projects = &drupal_static(__FUNCTION__, array());

instead of using statics you should use properties on the object instead.

+++ b/core/modules/update/lib/Drupal/update/UpdateCompareManager.phpundefined
@@ -0,0 +1,354 @@
+        if (config('update.settings')->get('check.disabled_extensions')) {
...
+        drupal_alter('update_projects', $projects);
...
+        \Drupal::keyValueExpirable('update')->setWithExpire('update_project_projects', $projects, 3600);

Please inject the various dependencies like the config factory, the module handler and the key value expirable service.

+++ b/core/modules/update/lib/Drupal/update/UpdateCompareManager.phpundefined
@@ -0,0 +1,354 @@
+      \Drupal::keyValueExpirable('update')->delete($key);
...
+      $projects = \Drupal::keyValueExpirable('update')->get($key);

more dependencies

+++ b/core/modules/update/lib/Drupal/update/UpdateFetchManager.phpundefined
@@ -0,0 +1,332 @@
+    $fetch_tasks = &drupal_static('_update_create_fetch_task', array());
+    if (empty($fetch_tasks)) {
+      $fetch_tasks = \Drupal::service('keyvalue')->get('update_fetch_task')->getAll();
+    }
+    if (empty($fetch_tasks[$project['name']])) {
+      $queue = \Drupal::queue('update_fetch_tasks');
+      $queue->createItem($project);
+      \Drupal::service('keyvalue')->get('update_fetch_task')->set($project['name'], $project);
+      $fetch_tasks[$project['name']] = REQUEST_TIME;
...
+    $queue = \Drupal::queue('update_fetch_tasks');
+    $end = time() + config('update.settings')->get('fetch.timeout');

Yeah another codeblock with a lot of dependencies and static variables, see above.

zengenuity’s picture

I've tried to fix all the issues mentioned by @dawehner. The only one I got totally stumped on was the batch and callables. I've never done that before, but after some Googling I had it working until I added the dependency injection stuff, which broke it.

What I found is that this would work, with a non-static fetchDataBatch method:

$batch = array(
  'operations' => array(
    array(array(new UpdateFetchController(), 'fetchDataBatch'), array()),
  ),
  'finished' => 'update_fetch_data_finished',
  ...

But, once I started injecting the UpdateFetchManager, it needs to be something like this:

$batch = array(
  'operations' => array(
    array(array(new UpdateFetchController($this->updateFetchManager), 'fetchDataBatch'), array()),
  ),
  'finished' => 'update_fetch_data_finished',
  ...

Whenever I ran it with the constructor parameter in there, I got an error:
Fatal error: Call to a member function prepare() on a non-object in /DRUPAL_ROOT/core/lib/Drupal/Core/Database/Connection.php on line 339

Was not sure what to do at that point, so I switched it back to the static method. I may have been totally off-base on this, though. So, if you can show me an example of what you mean, I will give it a try. BTW, the "finished" callback of the batch doesn't seem to support any of the new OO stuff at all. It has a function_exists() check in it that breaks both the static and non-static method callback.

Status: Needs review » Needs work

The last submitted patch, 1987888-update-manual-status-17.patch, failed testing.

zengenuity’s picture

Status: Needs work » Needs review
FileSize
82.5 KB
2.2 KB
126.3 KB

Locale module calls functions in update.compare.inc that have been migrated to UpdateCompareManager.php. I've switched the locale code to use the new service.

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateFetchController.phpundefined
@@ -0,0 +1,114 @@
+        array('\Drupal\update\Controller\UpdateFetchController::fetchDataBatch', array()),
...
+   * @param $context
...
+  public static function fetchDataBatch(&$context) {
+    $queue = \Drupal::queue('update_fetch_tasks');
...
+        if (\Drupal::service('update.fetch')->processFetchTask($item->data)) {

Can't you maybe pass in a full callable, so you can have a proper object with injected services? ... Let's typehint $context as array.

+++ b/core/modules/update/lib/Drupal/update/Tests/UpdateCoreUnitTest.phpundefined
@@ -7,12 +7,15 @@
-class UpdateCoreUnitTest extends UnitTestBase {
+class UpdateCoreUnitTest extends DrupalUnitTestBase {

Wow this is sad!

+++ b/core/modules/update/lib/Drupal/update/UpdateCompareManager.phpundefined
@@ -0,0 +1,875 @@
+   * @var \Drupal\Core\Extension\ModuleHandler
...
+   * @param \Drupal\Core\Extension\ModuleHandler $moduleHandler
...
+  public function __construct(ModuleHandler $moduleHandler, KeyValueExpirableFactory $keyValueExpirable) {

This should be all ModuleHandlerInterface

+++ b/core/modules/update/lib/Drupal/update/UpdateCompareManager.phpundefined
@@ -0,0 +1,875 @@
+   * @see processProjectInfo()
+   * @see calculateProjectData()
+   * @see projectStorage()
...
+   * @see update_get_available()
+   * @see getProjects()
+   * @see processProjectInfo()
+   * @see projectStorage()

Would be cool to have the full namespace in there.

+++ b/core/modules/update/lib/Drupal/update/UpdateCompareManager.phpundefined
@@ -0,0 +1,875 @@
+        $this->keyValueExpirable->get('update')->setWithExpire('update_project_projects', $this->projects, 3600);

Just an idea, we could store the actual instance on the object and not the factory.

+++ b/core/modules/update/lib/Drupal/update/UpdateCompareManager.phpundefined
@@ -0,0 +1,875 @@
+   * @param $projects
...
+  public function processProjectInfo(&$projects) {

Let's typehint as array

+++ b/core/modules/update/lib/Drupal/update/UpdateCompareManager.phpundefined
@@ -0,0 +1,875 @@
+          $project_data['status'] = UPDATE_REVOKED;
...
+          $project_data['status'] = UPDATE_NOT_SUPPORTED;
...
+          $project_data['status'] = UPDATE_NOT_FETCHED;

It is probably a good follow up to move this maybe on this class.

+++ b/core/modules/update/lib/Drupal/update/UpdateCompareManager.phpundefined
@@ -0,0 +1,875 @@
+   * @param $key
...
+  public function projectStorage($key) {

$key is probably a string.

+++ b/core/modules/update/lib/Drupal/update/UpdateCompareManager.phpundefined
@@ -0,0 +1,875 @@
+      $this->keyValueExpirable->get('update')->delete($key);
...
+      $projects = $this->keyValueExpirable->get('update')->get($key);

Another advantage of storing the factory would be that there is no requirement to ask for the instance all the time.

+++ b/core/modules/update/lib/Drupal/update/UpdateCompareManager.phpundefined
@@ -0,0 +1,875 @@
+    return array_intersect_key($info, drupal_map_assoc($whitelist));

drupal_map_assoc can be replaced by MapArray::copyValuesToKeys

+++ b/core/modules/update/lib/Drupal/update/UpdateFetchManager.phpundefined
@@ -0,0 +1,414 @@
+    $this->queue = $queue;
...
+      $this->queue->get('update_fetch_tasks')->createItem($project);
+      $this->keyValue->get('update_fetch_task')->set($project['name'], $project);
...
+    $end = time() + $this->config->get('update.settings')->get('fetch.timeout');
+    while (time() < $end && ($item = $this->queue->get('update_fetch_tasks')->claimItem())) {

I suggest to do the same thing as in the other class for all this different factories.

+++ b/core/modules/update/lib/Drupal/update/UpdateFetchManager.phpundefined
@@ -0,0 +1,414 @@
+        $data = \Drupal::httpClient()

So the http client should be injected.

+++ b/core/modules/update/lib/Drupal/update/UpdateFetchManager.phpundefined
@@ -0,0 +1,414 @@
+          ->get($url, array('Accept' => 'text/xml'))
+          ->send()

Seriously, guzzle rocks!

zengenuity’s picture

Status: Needs work » Needs review
FileSize
24.24 KB
126.93 KB

Okay, I've tried to address all the issues from #20: updating type hinting throughout these classes, switching over to storing the objects rather than the factories, and injecting the httpClient.

A couple things I wasn't able to do:

The callable for the batch callback. I don't know what to do here, but that's because I haven't passed a callable like this before. So, if someone can point me to an example, I will try to implement this. I did try a couple things described in #17, so check the note there about the problem I ran into.

The switch from UnitTestBase to DrupalUnitTestBase: I had to do this because dependency injection code doesn't work in UnitTestBase. Perhaps the tests should be rewritten to not depend on that, but again, I'm not sure what do here. UpdateFetchManager depends on a bunch of services, and I'm not sure I'm familiar enough with them to create them independently without getting them from the container. If there's an example somewhere, that would be helpful.

ParisLiakos’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
zengenuity’s picture

Status: Needs work » Needs review
FileSize
126.92 KB

Rerolled.

ParisLiakos’s picture

dipen chaudhary’s picture

Status: Needs review » Needs work

Needs to be rerolled, patch didn't apply.

zengenuity’s picture

Rerolled.

zengenuity’s picture

Status: Needs work » Needs review
star-szr’s picture

Status: Needs review » Needs work

Thanks for sticking with this @zengenuity! Needs another reroll.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +LONDON_2013_JULY
FileSize
65.36 KB

Re-roll...

Crell’s picture

  1. @@ -268,7 +269,7 @@ function testUpdateShowDisabledThemes() {
        * Tests updates with a hidden base theme.
        */
       function testUpdateHiddenBaseTheme() {
    -    module_load_include('compare.inc', 'update');
    +    $compareManager = new UpdateCompareManager($this->container->get('module_handler'), $this->container->get('keyvalue.expirable'));
     
         // Enable the subtheme.
    

    This looks like a great candidate for a PHPUnit test instead.

  2. @@ -24,20 +27,24 @@ class UpdateCoreUnitTest extends UnitTestBase {
       function testUpdateBuildFetchUrl() {
    +    $service = new UpdateFetchManager(
    +      $this->container->get('config.factory'),
    +      $this->container->get('queue'),
    +      $this->container->get('keyvalue'),
    +      $this->container->get('keyvalue.expirable'),
    +      $this->container->get('state'),
    +      $this->container->get('http_default_client'),
    +      new UpdateCompareManager($this->container->get('module_handler'), $this->container->get('keyvalue.expirable'))
    +    );
         //first test that we didn't break the trivial case
    

    Same. PHPUnit FTW.

  3. @@ -0,0 +1,664 @@
    +            'label' => t('Project not secure'),
    

    Translation should be injected.

  4. @@ -0,0 +1,427 @@
    +  public function __construct(ConfigFactory $config, QueueFactory $queue, KeyValueFactory $keyValue, KeyValueExpirableFactory $keyValueExpirable, KeyValueStoreInterface $state, ClientInterface $httpClient, UpdateCompareManager $updateCompareManager) {
    +    $this->updateConfig = $config->get('update.settings');
    +    $this->queue = $queue->get('update_fetch_tasks');
    +    $this->fetchTaskStore = $keyValue->get('update_fetch_task');
    +    $this->availableReleaseStore = $keyValueExpirable->get('update_available_releases');
    +    $this->updateStore = $keyValueExpirable->get('update');
    +    $this->state = $state;
    +    $this->updateCompareManger = $updateCompareManager;
    +    $this->httpClient = $httpClient;
    +  }
    

    This number of dependencies makes me wonder if this class shouldn't be broken up into separate pieces in the first place. It's feeling somewhat God-ish.

  5. @@ -0,0 +1,427 @@
    +    module_load_install('update');
    +    $status = update_requirements('runtime');
    

    With all the refactoring going on here anyway, let's not leave this procedural legacy around as it makes the class un-unit-testable. (Or at least not without hackery.)

Crell’s picture

Status: Needs review » Needs work
zengenuity’s picture

I re-rolled the patch and tried to address a couple of the points made by @Crell. I could use some direction on some of these:

1 & 2: I don't know exactly how to proceed with changing these tests to PHPUnit. Is there an example somewhere I can look at of a test that was converted?

3: Translation is now injected.

4: God-ish UpdateFetchManager: Yes, agreed, but I'm not experienced enough with core dev to determine how to break it up. If someone can give me a plan for it, I'm willing to try to break up the code.

5: Procedural code in UpdateCompareManager: So, I started this process. I've removed the two lines noted by @Crell, but to do so I needed to move procedural code into the UpdateCompareManager & UpdateFetchManager classes. That's probably okay, except now I have even more procedural functions called from the functions I moved. Some of these can be injected, I suspect, like whatever the replacements for l() and url() are. But, I decided to stop before I end up making another God-ish class with long list of injected classes. Finishing this up may depend on what is decided for #4.

zengenuity’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1987888-update-manual-status-32.patch, failed testing.

Crell’s picture

Assigned: zengenuity » dww
Status: Needs work » Needs review

Let's get dww's take on the refactoring here. (And maybe what parts should be punted.)

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.

zengenuity’s picture

Here's the minimal patch that just adds the route definition and controller class method.

Status: Needs review » Needs work

The last submitted patch, 1987888-update-manual-status-37.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
3.06 KB
2.53 KB

Thanks for jumping on this so quickly @zengenuity. That's not quite what we're doing with these though. Here's a patch with an interdiff so you can see what's being done.

xjm’s picture

Thanks @disasm! Let's also file a followup issue right away with the diff of the previous patch in #32 against this one so that the previous work is still there to iterate on later.

Edit: Just saw the bit in the summary about resetting the issue status to active. I'm not sure who added that (the revision log is tricksy) but I don't think that's the right tactic. We need to be able to tell when the conversions have been committed.

dawehner’s picture

FileSize
1.09 KB
2.65 KB

So yeah, let's do the full thing later.

This patch just removes the hook_menu entry completely.

Status: Needs review » Needs work
Issue tags: -Needs reroll, -WSCCI-conversion, -LONDON_2013_JULY

The last submitted patch, update-1987888-41.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#41: update-1987888-41.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs reroll, +WSCCI-conversion, +LONDON_2013_JULY

The last submitted patch, update-1987888-41.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review

DisplayBlockTest fails again i am looking at you @tim.plunkett

jibran’s picture

#41: update-1987888-41.patch queued for re-testing.

andypost’s picture

+1 RTBC. Just a minor nitpicks:

  1. +++ b/core/modules/update/lib/Drupal/update/Controller/UpdateController.php
    @@ -62,4 +63,15 @@ public function updateStatus() {
    +   *   A redirect response if the batch is progressive. No return value otherwise.
    

    needs wrapping around 80 chars

  2. +++ b/core/modules/update/lib/Drupal/update/Controller/UpdateController.php
    @@ -62,4 +63,15 @@ public function updateStatus() {
    +    module_load_include('fetch.inc', 'update');
    

    seems better to include the file itself without module_load_include() __DIR__ . '..\..

_12345678912345678’s picture

#41: update-1987888-41.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs reroll, +WSCCI-conversion, +LONDON_2013_JULY

The last submitted patch, update-1987888-41.patch, failed testing.

_12345678912345678’s picture

Failed when I re-tested it. :(

zengenuity’s picture

The simple route conversion were already committed as part of this commit: http://drupalcode.org/project/drupal.git/commit/ef2e45b0e86fc6863b8ee6d3...

Was the follow-up refactoring issue created?

ParisLiakos’s picture

lets keep it up here.#32 needs a reroll

JB13’s picture

Status: Needs work » Needs review
FileSize
2.14 KB

rerolling the patch.

pfrenssen’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll, -LONDON_2013_JULY

Some minor cleanups:

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateController.phpundefined
@@ -63,9 +64,12 @@ public function updateStatus() {
+   *   A redirect response if the batch is progressive. No return value otherwise.

Comment exceeds 80 characters.

+++ b/core/modules/update/update.routing.ymlundefined
@@ -93,3 +101,4 @@ update.confirmation_page:
     _permission: 'administer software updates'
     _access_update_manager: 'TRUE'
+

This adds an empty line to the end of the file.

disasm’s picture

Status: Needs work » Needs review
FileSize
62.28 KB

quick updates are in, reroll of #29. Bound to be some issues with everything that's happened in the last month, but this'll at least jump start this issue again.

The last submitted patch, drupal8.update-module.1987888-55.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

adding notification that this is a stub.

zengenuity’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
21.02 KB

Here's another pass at the refactoring task. (the stubs in the previous patches are already in)

This patch breaks the functions required for this callback into three classes to minimize external dependencies for each one:
UpdateFetcher: somebody else wrote this for another issue. It just wraps around Guzzle to fetch a single project's info. depends on ClientInterface
UpdateProcessor: does all the fetch queuing and processing the full list of projects. depends on UpdateFetcher and QueueFactory
UpdateManager: other functions that don't directly require the queue or fetcher. depends on UpdateProcessor and ModuleHandler

Status: Needs review » Needs work

The last submitted patch, 57: 1987888-update-manual-status-57.patch, failed testing.

zengenuity’s picture

Status: Needs work » Needs review
FileSize
41.74 KB

Forgot to include the new files in the patch. Re-testing.

kim.pepper’s picture

Crell’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/update/lib/Drupal/update/UpdateManager.php
    @@ -0,0 +1,275 @@
    +    $this->tempStore = \Drupal::keyValueExpirable('update');
    +    $this->availableReleasesTempStore = \Drupal::keyValueExpirable('update_available_releases');
    

    These should be injected, too.

  2. +++ b/core/modules/update/lib/Drupal/update/UpdateProcessor.php
    @@ -0,0 +1,285 @@
    +    $this->tempStore = \Drupal::keyValueExpirable('update');
    +    $this->fetchTaskStore = \Drupal::keyValue('update_fetch_task');
    +    $this->availableReleasesTempStore = \Drupal::keyValueExpirable('update_available_releases');
    

    As above. Inject these.

    It looks like this is a lot of dependencies in this class. That's a hint that it may need to get split up into different objects.

  3. +++ b/core/modules/update/lib/Drupal/update/UpdateProcessor.php
    @@ -0,0 +1,285 @@
    +    // Whether this worked or not, we did just (try to) check for updates.
    +    \Drupal::state()->set('update.last_check', REQUEST_TIME + $request_time_difference);
    

    Inject this. \Drupal should *never* appear inside a class.

This issue has basically morphed into "objectify the update system". Should it be retitled?

kim.pepper’s picture

I'll have a go at this.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
43.75 KB
13.61 KB

Fixes procedural calls to \Drupal-> by injecting some more services. Also a couple of docs clean ups.

Crell’s picture

Assigned: kim.pepper » dww

That's much better, thanks.

Given the scope of this issue, I think Derek should weigh in before it gets RTBCed. Derek, any feedback before this is commitable?

dww’s picture

Assigned: dww » Unassigned

I'll have to find someone in IRC who can explain to me what a "new style controller" means. I'm so far out of the loop on D8 I don't have any frame of reference for even starting to review this patch. I mean, I can go through it like an ignorant monkey and point out WTFs like this:

+      $container->get('module_handler'),
+      $container->get('update.manager')

Why do we sometimes separate with a _ and other times with a .?

But I doubt that's the kind of review you're looking for. There's nothing in the summary to tell me what problem this patch is trying to solve, what thing could be accomplished once it was committed that's not possible now, etc.

Thanks for asking, though. :) But, if anyone's more familiar with D8 and what's happening wants to RTBC, please do. No D8 core issue should be held up waiting my blessings at this point...

Cheers,
-Derek

zengenuity’s picture

Assigned: Unassigned » zengenuity

Assigning this back to myself because I'm willing to keep working on this. I know there needs to be a second pass through update.module, update.fetch.inc and update.compare.inc to refactor some additional functions that could end up using the new classes, but I posted my latest patch hoping for some feedback about whether my overall approach was reasonable before moving forward.

This is my first significant core patch, so I'm not on top of what everyone else is doing and whether the changes in this patch are going to be made redundant by somebody else's work. I'm happy to keep working on this, but I just don't want to end up wasting time if it can't be used.

Crell’s picture

The last submitted patch, 63: 1987888-update-manual-status-63.patch, failed testing.

The last submitted patch, 63: 1987888-update-manual-status-63.patch, failed testing.

kim.pepper’s picture

Re-roll

Crell’s picture

Aside from the need to reroll, I was fairly OK with where this patch was in #63. I just wanted someone who knew the update system to verify it. I guess #65 is as much as we're going to get, though.

I suggest we get this committed, and then we can open a normal follow-up to break up the classes even more if someone is interested in doing so. That's not release-blocking, though, and IMO not an API change so it's not pressing.

Crell’s picture

Status: Needs review » Reviewed & tested by the community
Related issues: +#2167779: Break up Update system classes

Follow-up filed: #2167779: Break up Update system classes Thanks, everyone!

zengenuity: If you're interested, you can have dibs on the follow-up issue. I'm fairly certain no one else is working on it yet since I just filed it. :-)

ParisLiakos’s picture

+++ b/core/modules/update/lib/Drupal/update/UpdateProcessor.php
@@ -0,0 +1,311 @@
+class UpdateProcessor {

+++ b/core/modules/update/lib/Drupal/update/UpdateManager.php
@@ -0,0 +1,298 @@
+class UpdateManager {

those classes should get interfaces at least

kim.pepper’s picture

Extracted interfaces for UpdateProcessor and UpdateFetcher.

ParisLiakos’s picture

Thanks! :)

  1. +++ b/core/modules/update/lib/Drupal/update/UpdateManager.php
    @@ -0,0 +1,298 @@
    +class UpdateManager {
    

    oh fetcher didnt have an interface as well..but we still need one for manager ;)

  2. +++ b/core/modules/update/lib/Drupal/update/UpdateProcessor.php
    @@ -0,0 +1,311 @@
    +   * Adds a task to the queue for fetching release history data for a project.
    ...
    +  public function createFetchTask($project) {
    ...
    +   * Attempts to drain the queue of tasks for release history data to fetch.
    ...
    +  public function fetchData() {
    ...
    +   * Processes a task to fetch available update data for a single project.
    ...
    +  public function processFetchTask($project) {
    ...
    +   * Retrieves the number of items in the update fetch queue.
    ...
    +  public function numberOfQueueItems() {
    ...
    +   * Claims an item in the update fetch queue for processing.
    ...
    +  public function claimQueueItem() {
    ...
    +   * Deletes a finished item from the update fetch queue.
    ...
    +  public function deleteQueueItem($item) {
    

    we can and should use inheritdocs here now

kim.pepper’s picture

Thanks for the review Paris.

Here's a patch which fixes those issues. I also fixed a bit of docblock formatting.

ParisLiakos’s picture

cool, thanks, interdiff looks good.
lets get this in and move on #2167779: Break up Update system classes

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed cc1892d and pushed to 8.x. Thanks!

Noticed #2181101: Update settings tab not visible during manual testing.

Fixed a small spelling mistake during commit.

diff --git a/core/modules/update/lib/Drupal/update/UpdateProcessor.php b/core/modules/update/lib/Drupal/update/UpdateProcessor.php
index 9f90126..611dce5 100644
--- a/core/modules/update/lib/Drupal/update/UpdateProcessor.php
+++ b/core/modules/update/lib/Drupal/update/UpdateProcessor.php
@@ -54,7 +54,7 @@ class UpdateProcessor implements UpdateProcessorInterface {
   protected $fetchTaskStore;

   /**
-   * Update avaiable releases store
+   * Update available releases store
    *
    * @var \Drupal\Core\KeyValueStore\KeyValueStoreExpirableInterface
    */

Status: Fixed » Closed (fixed)

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