Support from Acquia helps fund testing for Drupal Acquia logo

Comments

shnark’s picture

Assigned: Unassigned » shnark
shnark’s picture

Status: Active » Needs review
FileSize
3.17 KB
Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateController.php
@@ -0,0 +1,42 @@
+/**
+ * @file
+ * Contains the controller for the update module.

For a class, the @file directive is fairly simplistic. It's just the name of the class, like so:

@file
Contains \Drupal\update\Controller\UpdateController

(Yeah, it's silly, but that's what the coding standards say.)

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateController.php
@@ -0,0 +1,42 @@
+  function update_status() {

Coding standards for methods are to use lowerCamel. So this should be:

public function updateStatus() {

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateController.php
@@ -0,0 +1,42 @@
+      module_load_include('inc', 'update', 'update.compare');

Hm, this function. Actually, this can now be an injected service. Let's go ahead and convert that now, for practice. :-)

There's a service called module_handler, which can be injected via create(). Then there's a method on that service called loadInclude() that replaces this function.

Really, the contents of update.compare.inc should get turned into a service. That's a big complex, though, so we can do that in a follow-up.

shnark’s picture

Status: Needs work » Needs review
FileSize
1.41 KB
3.18 KB

I made all the changes that Crell recommended in comment #3, except for the last one, which I will work on next.

YesCT’s picture

Status: Needs review » Needs work

I think we can look at #1938296: Convert book_admin_overview and book_render to a new-style Controller again for an example.

--- /dev/null
+++ b/core/modules/book/lib/Drupal/book/BookManager.phpundefined

@@ -0,0 +1,83 @@
+<?php
+/**
+ * @file
+ * Contains \Drupal\book\BookManager.
+ */
+
+namespace Drupal\book;
+
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Drupal\Core\Database\Connection;
+use PDO;
+
+/**
+ * Book Manager Service.
+ */
+class BookManager {
+
+  /**
+   * Database Service Object.

etc.

look at this Manager bit for an example.

@Crell any more pointers than that?

---
moving it back to needs work to add the manager bit.

Crell’s picture

The book issue needed to make a new service. This issue doesn't. It just needs to use an existing service. So the only change would be in the create() method, where you have to reference the service we're injecting, and then replace module_load_include() with the equivalent from that service. There's no additional class like BookManager that would be needed.

YesCT’s picture

ah! ok.

YesCT’s picture

Assigned: shnark » Unassigned

@EllaTheHarpy if you want to do this, just reassign it to yourself when you have time. Otherwise, this is up for grabs.
It's on my list to try, but I dont have time right away either... I'll check back in on this later.

sandhya.m’s picture

Assigned: Unassigned » sandhya.m
Status: Needs work » Needs review
FileSize
8.19 KB

Used moduleHandler to use loadInclude() for the inc files in UpdateController.php

dawehner’s picture

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateController.phpundefined
@@ -0,0 +1,65 @@
+
+use Drupal\Core\ControllerInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Drupal\Core\Extension\ModuleHandler;

let's order them.

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateController.phpundefined
@@ -0,0 +1,65 @@
+   * @var \Drupal\Core\Entity\EntityManager
...
+   * @param \Drupal\Core\Extension\ModuleHandler $module_handler

Should be the interface of the module handler :)

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateController.phpundefined
@@ -0,0 +1,65 @@
+  public function __construct(ModuleHandler $module_handler) {

Let's move it on top of create

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateController.phpundefined
@@ -0,0 +1,65 @@
+      return theme('update_report', array('data' => $data));
...
+      return theme('update_report', array('data' => _update_no_data()));

I guess it's in the scope of this issue to return a build array.

alexpott’s picture

Issue tags: +LONDON_2013_APRIL

Tagging...

sandhya.m’s picture

Changes suggested by dawehner : done and added to the new patch

alexpott’s picture

Issue tags: +LONDON_2013_APRIL

tagging

dawehner’s picture

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateController.phpundefined
@@ -0,0 +1,68 @@
+   * @var \Drupal\Core\Entity\EntityManager

Should be here marked as ModuleHandlerInterface as well.

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateController.phpundefined
@@ -0,0 +1,68 @@
+  /**
+   * Implements \Drupal\Core\ControllerInterface::create().
+   *
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
+   *   The service container this object should use.
+   */

You can use just {@inheritdoc}

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateController.phpundefined
@@ -0,0 +1,68 @@
+   * @return string
+   *   A HTML-formatted string with the update status of projects.

... That's not a string, but a build array which is returned.

sandhya.m’s picture

Updated the code as per the comments on #14

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Great!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c7d74b3 and pushed to 8.x. Thanks!

@sandhya.m you need to set up your environment to make your patches a little easier to apply... see http://stackoverflow.com/questions/13675782/git-shell-in-windows-patchs-... / or you can use notepad++ to convert to utf-8 and unix line endings.

tim.plunkett’s picture

Status: Fixed » Needs review
FileSize
1.69 KB
3.05 KB

#1953800: Make the database connection serializable means we cannot yet inject module_handler.

This was rolled back.

Status: Needs review » Needs work

The last submitted patch, update-1938822-18.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.29 KB
2.22 KB

Hmmm.... it's because update_theme relied on the menu to include update.report.inc :D

So the moduleHandler injection is fine... afaik the serialisation would only happen on a form conversion... when the controller gets added to form_state.

I have no idea how #15 passed... maybe because of the encoding testbot couldn't actually properly apply the patch but it thought it did.... very very weird.

tim.plunkett’s picture

Oh that makes a bit more sense. I just get twitchy when I see module_handler injected since I wasted a couple hours on it.

YesCT’s picture

FileSize
847 bytes
4.29 KB

I checked that the @param removed when Implements \... was replaced with {@inheritdoc} was not adding info.. and it was just the same @param that is in core/lib/Drupal/Core/ControllerInterface.php

So no loss of info.

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateController.phpundefined
@@ -0,0 +1,65 @@
+ * @file
+ * Contains \Drupal\update\Controller\UpdateController

just missing period.

The summary should be one line of up to 80 characters ending in ".".
http://drupal.org/node/1354#file

+++ b/core/modules/update/lib/Drupal/update/Controller/UpdateController.phpundefined
@@ -0,0 +1,65 @@
+    $build  = array(

extra white space.

This looks rtbc to me. I compared it to what was done in #1938296: Convert book_admin_overview and book_render to a new-style Controller.

but I just changed some stuff, so someone else can rtbc it.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Let's do.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a90da0d and pushed to 8.x. Thanks!

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