CommentFileSizeAuthor
#90 1974408-aggregator-source-controller-90.patch23.85 KBkim.pepper
#90 interdiff.txt1.73 KBkim.pepper
#87 1974408-aggregator-source-controller-87.patch23.84 KBkim.pepper
#87 interdiff.txt3.99 KBkim.pepper
#81 drupal8.aggregator-module.1974408-80.patch24.82 KBdisasm
#81 interdiff.txt7.13 KBdisasm
#76 interdiff.txt9.57 KBdisasm
#75 drupal8.aggregator-module.1974408-75.patch24.97 KBdisasm
#75 interdiff.txt958 bytesdisasm
#73 1974408-aggregator-source-controller-73.patch24.55 KBkim.pepper
#73 interdiff.txt958 byteskim.pepper
#72 drupal8.aggregator-module.1974408-72.patch24.65 KBdisasm
#72 interdiff.txt5.36 KBdisasm
#69 1974408-aggregator-source-controller-69.patch23.76 KBkim.pepper
#69 interdiff.txt812 byteskim.pepper
#67 drupal8.aggregator-module.1974408-67.patch23.83 KBdisasm
#67 interdiff.txt17.96 KBdisasm
#65 drupal8.aggregator-module.1974408-65.patch19.96 KBdisasm
#65 interdiff.txt14.09 KBdisasm
#62 1974408-aggregator-source-controller-62.patch13.88 KBkim.pepper
#62 interdiff.txt10.51 KBkim.pepper
#54 1974408-aggregator-source-controller-54.patch11.55 KBParisLiakos
#54 interdiff.txt904 bytesParisLiakos
#52 1974408-aggregator-source-controller-52.patch11.45 KBParisLiakos
#52 interdiff.txt3.4 KBParisLiakos
#49 1974408-aggregator-source-controller-49.patch12.07 KBParisLiakos
#49 interdiff.txt920 bytesParisLiakos
#46 1974408-aggregator-source-controller-46.patch12.21 KBParisLiakos
#46 interdiff.txt6.99 KBParisLiakos
#45 1974408-aggregator-source-controller-45.patch8.13 KBkim.pepper
#45 interdiff.txt632 byteskim.pepper
#43 1974408-aggregator-source-controller-42.patch8.11 KBkim.pepper
#43 interdiff.txt1.63 KBkim.pepper
#38 1974408-aggregator-source-controller-38.patch8.15 KBkim.pepper
#38 interdiff.txt1.6 KBkim.pepper
#33 1974408-aggregator-source-controller-33.patch8.14 KBkim.pepper
#33 interdiff.txt4.54 KBkim.pepper
#29 1974408-aggregator-source-controller-29.patch7.82 KBkim.pepper
#29 interdiff.txt6.75 KBkim.pepper
#26 1974408-aggregator-source-controller-26.patch6.71 KBkim.pepper
#26 interdiff.txt755 byteskim.pepper
#24 aggregator-page-source-controller-1974408-24.patch6.71 KBpwieck
#18 aggregator-page-source-controller-1974408-18.patch7.16 KBtheMusician
#11 aggregator-page-source-controller-1974408-11.patch7.65 KBkshama_deshmukh
#9 2-9-interdiff.txt1.77 KBalexpott
#9 aggregator-page-source-controller-1974408-9.patch4.88 KBalexpott
#2 aggregator-page-source-controller-1974408-2.patch3.38 KBkshama_deshmukh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kshama_deshmukh’s picture

Assigned: Unassigned » kshama_deshmukh
kshama_deshmukh’s picture

kshama_deshmukh’s picture

Status: Active » Needs review
kshama_deshmukh’s picture

aggregator_page_source() is also used by aggregator_page_source_form(). Hence, needs to remove aggregator_page_source() from aggregator.pages.inc while conversion of aggregator_page_source_form() to WSCCI.

alexpott’s picture

Issue tags: +LONDON_2013_APRIL

Tagging

alexpott’s picture

Issue tags: +Stalking Crell
+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -200,12 +200,9 @@ function aggregator_menu() {
   $items['aggregator/sources/%aggregator_feed'] = array(
-    'title callback' => 'entity_page_label',
+    'title callback' => '_aggregator_page_label',
     'title arguments' => array(2),
-    'page callback' => 'aggregator_page_source',
-    'page arguments' => array(2),
-    'access arguments' => array('access news feeds'),
-    'file' => 'aggregator.pages.inc',
+    'route_name' => 'aggregator_page_source',
   );
   $items['aggregator/sources/%aggregator_feed/view'] = array(
     'title' => 'View',
@@ -251,6 +248,26 @@ function aggregator_menu() {

@@ -251,6 +248,26 @@ function aggregator_menu() {
 }
 
 /**
+ * Title callback: Returns a title for Aggregator feed view pages.
+ *
+ * @param int $fid
+ *   Aggregator Feed Id for which to generate a label.
+ * @param $langcode
+ *   (optional) The language code of the language that should be used for
+ *   getting the label. If set to NULL, the entity's default language is
+ *   used.
+ *
+ * @return
+ *   The label of the Aggregator Feed, or NULL if there is no label defined.
+ *
+ * @see aggregator_menu()
+ */
+function _aggregator_page_label($fid, $langcode = NULL) {
+  $feed = aggregator_feed_load($fid);
+  return entity_page_label($feed, $langcode);
+}

Once we converted the hook_menu to use the route %aggregator_feed was no longer autoloaded by aggregator_feed_load. So kshama has introduced a new title callback to handle this. This (to me) appears to be a regression. Adding the Stalking Crell tag to get an opinion :)

dawehner’s picture

Status: Needs review » Needs work

The last submitted patch, aggregator-page-source-controller-1974408-2.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.88 KB
1.77 KB

Fixed up patch in #2 to investigate fixes for #1981644 ... none found so far

Status: Needs review » Needs work

The last submitted patch, aggregator-page-source-controller-1974408-9.patch, failed testing.

kshama_deshmukh’s picture

Status: Needs work » Needs review
FileSize
7.65 KB

Fixed failed patch. Also moved used functions from aggregator.pages.inc to controller and removed usage of module handler.

ParisLiakos’s picture

here is i think a better way to deal with page_callbacks for now? #1978166: Convert block/add/{%custom_block_type} to Controller

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -56,4 +70,106 @@ public function feedAdd() {
+  protected function _buildPageList($items, $feed_source = '') {

no need for _ prefix, it is already marked protected

ParisLiakos’s picture

Status: Needs review » Needs work

drupal_set_title solution is the way to go for now. see the issue linked above for example
also

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -56,4 +70,106 @@ public function feedAdd() {
+  public function loadFeedItems($type, $data = NULL, $limit = 20) {

lets use the procedural function here, this method is definitely not a method for a controller

kim.pepper’s picture

Status: Needs work » Needs review
Issue tags: -Stalking Crell, -WSCCI-conversion, -LONDON_2013_APRIL

Status: Needs review » Needs work
Issue tags: +Stalking Crell, +WSCCI-conversion, +LONDON_2013_APRIL

The last submitted patch, aggregator-page-source-controller-1974408-11.patch, failed testing.

kim.pepper’s picture

Issue tags: +Needs reroll
theMusician’s picture

Working on re-rolling this right now.

theMusician’s picture

Here is a re-roll of the last patch. I needed to include the _ prefix mentioned in comment #12 or the aggregator module whitescreens. The PHP log reports:

PHP Fatal error: Call to undefined method Drupal\aggregator\Routing\AggregatorController::buildPageList() in /Applications/MAMP/htdocs/drupal8/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.php on line 117

Keeping the _ prefix prevents the issue.

theMusician’s picture

Status: Needs work » Needs review

Ugh...forgot to set for review.

dawehner’s picture

Can you explain why entity_page_label does not work here?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -98,6 +99,110 @@ public function feedAdd() {
+    return $this->_buildPageList($items, $feed_source);

No need to write _ in front of it.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.phpundefined
@@ -98,6 +99,110 @@ public function feedAdd() {
+  public function loadFeedItems($type, $data = NULL, $limit = 20) {

Couldn't you use entity query for that instead of manually querying the database.

ParisLiakos’s picture

lets just do, what we did with the pageLast() method here. most of those functions in aggregator.pages.inc will look very different or die once #15266: Replace aggregator category system with taxonomy is in..so just add a @todo above them

Gaelan’s picture

Issue tags: -Needs reroll
star-szr’s picture

Assigned: kshama_deshmukh » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs another reroll, AggregatorController got moved.

Checking patch core/modules/aggregator/aggregator.module...
Hunk #1 succeeded at 191 (offset -2 lines).
Hunk #2 succeeded at 239 (offset -2 lines).
Checking patch core/modules/aggregator/aggregator.routing.yml...
Checking patch core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.php...
error: core/modules/aggregator/lib/Drupal/aggregator/Routing/AggregatorController.php: No such file or directory
pwieck’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.71 KB

Reroll to current head.

Only changed:
+ return $this->_buildPageList($items, $feed_source);
TO
+ return $this->buildPageList($items, $feed_source);

Refer to #20 and #21 for further needed changes

Status: Needs review » Needs work

The last submitted patch, aggregator-page-source-controller-1974408-24.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
755 bytes
6.71 KB

@pwieck you forgot to change the actual method name in #24.

Status: Needs review » Needs work

The last submitted patch, 1974408-aggregator-source-controller-26.patch, failed testing.

dawehner’s picture

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -100,6 +101,110 @@ public function feedAdd() {
+  public function loadFeedItems($type, $data = NULL, $limit = 20) {

It doesn't seem helpful to have this method to be public as this is just the controller. On the longrun there should be a separate service for it?

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
6.75 KB
7.82 KB

Thanks for the review @dawehner.

I've fixed the broken tests in #26 and moved loadFeedItems() to the ItemStorageController interface and class.

dawehner’s picture

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -242,6 +239,24 @@ function aggregator_menu() {
+ * @param int $fid
+ *   Aggregator Feed Id for which to generate a label.
...
+function _aggregator_page_label($fid, $langcode = NULL) {

So as we don't have the load callbacks anymore the $fid is not upcasted anymore? Maybe just remove the placeholder in the uri of the hook_menu entry

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -242,6 +239,24 @@ function aggregator_menu() {
+ * @param $langcode
...
+ * @return string The label of the Aggregator Feed, or NULL if there is no label
+ * defined.@see aggregator_menu()

This documentation is missed up a bit.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -100,6 +101,53 @@ public function feedAdd() {
+   * @param \Drupal\aggregator\Plugin\Core\Entity\Feed $aggregator_feed
...
+  public function viewFeed(Feed $aggregator_feed = NULL) {

Let's use the FeedInterface instead.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -100,6 +101,53 @@ public function feedAdd() {
+   * @param \Drupal\aggregator\Plugin\Core\Entity\Item $items
...
+  protected function buildPageList($items, $feed_source = '') {

ItemInterface

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -100,6 +101,53 @@ public function feedAdd() {
+      $build['pager']['#markup'] = theme('pager');

Should be just a render array as well.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageController.phpundefined
@@ -112,4 +112,44 @@ public function saveCategories(Item $item) {
+    switch ($type) {
+      case 'sum':
+        $query = $this->database->select('aggregator_item', 'i');
+        $query->join('aggregator_feed', 'f', 'i.fid = f.fid');
+        $query->fields('i', array('iid'));
+        break;
+      case 'source':
+        $query = $this->database->select('aggregator_item', 'i');
+        $query
+          ->fields('i', array('iid'))
+          ->condition('i.fid', $data->id());
+        break;
+      case 'category':
+        $query = $this->database->select('aggregator_category_item', 'c');
+        $query->leftJoin('aggregator_item', 'i', 'c.iid = i.iid');
+        $query->leftJoin('aggregator_feed', 'f', 'i.fid = f.fid');
+        $query
+          ->fields('i', array('iid'))
+          ->condition('cid', $data->cid);
+        break;
+    }
+
+    $result = $query
+      ->extend('Drupal\Core\Database\Query\PagerSelectExtender')
+      ->limit($limit)
+      ->orderBy('i.timestamp', 'DESC')
+      ->orderBy('i.iid', 'DESC')
+      ->execute()
+      ->fetchCol();
+
+    $feed_items = $this->load($result);
+

should/could we not just make this a entity query?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageControllerInterface.phpundefined
@@ -41,4 +41,29 @@ public function deleteCategories(array $entities);
+   * @param $type
...
+   * @param $data
...
+   * @param $limit
...
+   * @return mixed An array of the feed items.

Needs types.

Status: Needs review » Needs work
Issue tags: -Stalking Crell, -WSCCI-conversion, -LONDON_2013_APRIL

The last submitted patch, 1974408-aggregator-source-controller-29.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
Issue tags: +Stalking Crell, +WSCCI-conversion, +LONDON_2013_APRIL
kim.pepper’s picture

Fixed the documentation issues, but have a few more comments.

@dawehner

So as we don't have the load callbacks anymore the $fid is not upcasted anymore? Maybe just remove the placeholder in the uri of the hook_menu entry

Sorry, I'm not entirely clear on what you mean here.

should/could we not just make this a entity query?

Discussed this with @dawehner in IRC and agreed to leave it in ItemStorageController as is for now.

Should be just a render array as well.

Not sure what needs to be done to convert that.

kim.pepper’s picture

Issue tags: +#pnx-sprint

Tagging

ParisLiakos’s picture

about the pager thing:
it should be

$build['pager'] = array('#theme' => 'pager');

instead of

$build['pager']['#markup'] = theme('pager');
ParisLiakos’s picture

dawehner’s picture

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -242,6 +239,27 @@ function aggregator_menu() {
+function _aggregator_page_label($fid, $langcode = NULL) {
+  $feed = aggregator_feed_load($fid);
+  return ($feed) ? entity_page_label($feed, $langcode) : '';
+}

This change might be needed because of #2023795: REGRESSION: hook_local_actions doesn't use title callback

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageControllerInterface.phpundefined
@@ -41,4 +41,30 @@ public function deleteCategories(array $entities);
+   * @param FeedInterface|array $data

FeedInterface could have the full namespace, ... mentioning as you already have to do something.

kim.pepper’s picture

Fixed #35 and doc change in #37.

I'm still not sure if I can do anything about the title callback just yet? Is this a follow up?

Kim

Status: Needs review » Needs work

The last submitted patch, 1974408-aggregator-source-controller-38.patch, failed testing.

kim.pepper’s picture

Looks like I messed up the render array stuff.

kim.pepper’s picture

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -101,6 +103,53 @@ public function feedAdd() {
+      $build['pager']['#theme'] = 'pager';

Is this right?

kim.pepper’s picture

Issue tags: +#pnx-sprint, +WSCCI-conversion

Double-post

kim.pepper’s picture

Status: Needs work » Needs review
Issue tags: -#pnx-sprint, -WSCCI-conversion
FileSize
1.63 KB
8.11 KB

I think I have the changes @dawehner pointed out in #30 including the title callback and replacing theme().

ParisLiakos’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -101,6 +103,53 @@ public function feedAdd() {
+    $build = array(
...
+    $build['feed_source'] = is_array($feed_source) ? $feed_source : array('#markup' => $feed_source);
...
+      $build['items'] = $this->entityManager->getRenderController(reset($items)->entityType())
...
+    return drupal_render($build);

so if the pager does not work in the #type => 'container', maybe add one more level to it?
eg

$build['wrapper'];// this is the #type => container
$build['wrapper']['feed_source'];
$build['wrapper']['items'];
$build['pager'];

havent tested, but i dont like the drupal_render there tbh

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
632 bytes
8.13 KB

Re-roll, and removed drupal_render as per #44.

ParisLiakos’s picture

i moved the whole thing to drupal_set_title() as originally proposed in #13
Also, since we are adding the buildPageList and the method in Item storage controller, lets cleanup the controller:)

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me!

YesCT’s picture

Issue tags: -Needs reroll +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

ParisLiakos’s picture

lets fix the use statements too

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1974408-aggregator-source-controller-49.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -189,12 +189,8 @@ function aggregator_menu() {
-    'title callback' => 'entity_page_label',
-    'title arguments' => array(2),
...
+    'title' => 'View',

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -20,6 +20,7 @@
+  drupal_set_title($feed->label());

'title callback' was broken for routes for a while but is fixed again, this can be reverted

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -189,12 +189,8 @@ function aggregator_menu() {
-    'page callback' => 'aggregator_page_source',
-    'page arguments' => array(2),

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -20,6 +20,7 @@
 function aggregator_page_source(Feed $feed) {

Also, is this still needed?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -70,14 +63,11 @@ class AggregatorController implements ControllerInterface {
     $this->entityManager = $entity_manager;

@@ -89,7 +79,6 @@ public static function create(ContainerInterface $container) {
       $container->get('plugin.manager.entity'),

@@ -111,6 +100,54 @@ public function feedAdd() {
+    $feed_source = $this->entityManager->getRenderController($aggregator_feed->entityType())
...
+    $items = $this->entityManager
+      ->getStorageController('aggregator_item')

I know we already have the entity manager injected, but can we please at least inject the storage controller, and maybe the render controller as well? Better typehints++

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -111,6 +100,54 @@ public function feedAdd() {
+      $build['items'] = $this->entityManager->getRenderController(reset($items)->entityType())

Same here with the render controller. Also, wouldn't we already know the entity type?

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
3.4 KB
11.45 KB

Also, is this still needed?

Yeap, it is called by aggregator_page_source_form()..

i fixed the title callbacks..also failures..i have no clue how this passed in the first time.

About the typehints, while i agree, and i started doing it, the constructor became unreadable (4 extra arguments)..this is a result of falsely putting everything in one controller..i would rather split the controller in 2: one for feeds one for items, one for categories and fix the typehints/arguments..but probably not in this issue? right?

edit: this is actually a mess, we cant split it, all of them would need more or less the same things:/. i ll have to think a bit more about this, but, not in this issue anyway

kim.pepper’s picture

Status: Needs review » Needs work

Looking good. One minor nitpick.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -70,14 +63,11 @@ class AggregatorController implements ControllerInterface {
    * @param \Drupal\Core\Config\ConfigFactory $config_factory
    *   The config factory.
-   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
-   *   The module handler.
    */
-  public function __construct(EntityManager $entity_manager, Connection $database, ConfigFactory $config_factory, ModuleHandlerInterface $module_handler, PathBasedGeneratorInterface $url_generator) {

Missing UrlGenerator docs.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
904 bytes
11.55 KB

added url generator docs

kim.pepper’s picture

Status: Needs review » Reviewed & tested by the community

Assuming the bot comes back green. This has been through a number of reviews, so putting back to RTBC

kim.pepper’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

So let's convert aggregator_page_source_form here as well... so we can get rid of aggregator_page_source() it makes no sense to be maintaining duplicate code...

Also considering this patch is copying the code from aggregator_load_feed_items() to the storage controller we have the same problem here... we should have duplicate methods that do the same thing - that's a recipe for disaster... that means three choices...

  1. we need to make aggregator_load_feed_items() a wrapper round the new storage controller method
  2. convert aggregator_page_category to use the storage controller
  3. rollback the method on the controller

I think I like option 2 as we can just Drupal::service in aggregator_page_category() and we can get rid of aggregator_load_feed_items

kim.pepper’s picture

Assigned: Unassigned » kim.pepper
kim.pepper’s picture

@alexpott FYI there is a separate issue for #2040199: Convert aggregator_page_category_form and aggregator_page_source_form to a Controller which does away with aggregator_load_feed_items() and there is also a separate issue #2043581: Move aggregator_load_feed_items to the ItemStorageController, retain procedural wrapper

kim.pepper’s picture

Issue summary: View changes

Link to meta issue

kim.pepper’s picture

Status: Needs work » Postponed

Blocked by #2040199: Convert aggregator_page_category_form and aggregator_page_source_form to a Controller as the currently forms just wrap these page callbacks. Once that is in, we can safely combine this and #1974394: Convert aggregator_page_category() to a Controller and move the common code to the controller.

kim.pepper’s picture

Issue summary: View changes

related issues

kim.pepper’s picture

Status: Postponed » Active

unblocked

kim.pepper’s picture

Status: Active » Needs review
FileSize
10.51 KB
13.88 KB
  • Combines #1974394: Convert aggregator_page_category() to a Controller
  • Removes procedural callbacks in aggregator.pages.inc
  • Moves shared code in _aggregator_page_list() to a protected method AggregatorController::buildPageList()
  • Refactors some calls to deprecated functions to use CategoryStorageController
jibran’s picture

  1. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
    @@ -111,6 +120,69 @@ public function feedAdd() {
    +   * @return string
    

    This should be array.

  2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
    @@ -306,13 +366,14 @@ public function sources() {
    +      $feed->url = $this->urlGenerator->generateFromRoute('aggregator_feed_view', array('aggregator_feed' => $feed->id()));
    

    nice.

Status: Needs review » Needs work

The last submitted patch, 1974408-aggregator-source-controller-62.patch, failed testing.

disasm’s picture

Switching to ControllerBase. This also fixes the __construct errors with having the wrong type hint for UrlGeneratorInterface, since we're getting that from ControllerBase now.

disasm’s picture

Status: Needs work » Needs review
disasm’s picture

forgot to switch to $this->t() this one resolves that.

Status: Needs review » Needs work

The last submitted patch, drupal8.aggregator-module.1974408-67.patch, failed testing.

kim.pepper’s picture

Fixes an issue with $this->redirect()

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1974408-aggregator-source-controller-69.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
5.36 KB
24.65 KB

Attached patch replaces drupal_set_title with #title on the build array. It also renames $database to $connection. As for the test failures, I've done manual testing and can't replicate the issues. I see them though when I run simpletest.

kim.pepper’s picture

Checked with dawehner in IRC and we still need to provide menu title callbacks for the menu links. Setting the page title doesn't do this for us.

Fixes tests locally.

ParisLiakos’s picture

Cool, its green! thanks guys

  1. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
    @@ -7,78 +7,46 @@
    +class AggregatorController extends ControllerBase implements ContainerInjectionInterface {
    

    i dont know about that. what is the point of implementing ContainerInjectionInterface if you extend from ControllerBase?
    you can have access to *anything* with $this->container

    No need for the create() hack, really

  2. +++ b/core/modules/aggregator/aggregator.pages.inc
    @@ -94,36 +51,6 @@ function aggregator_load_feed_items($type, $data = NULL, $limit = 20) {
    -function _aggregator_page_list($items, $op, $feed_source = '') {
    

    Yay!

  3. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
    @@ -101,13 +66,76 @@ public static function create(ContainerInterface $container) {
    +    $feed = $this->entityManager()
           ->getStorageController('aggregator_feed')
    ...
    +    return $this->entityManager()->getForm($feed);
    ...
    +    $feed_source = $this->entityManager()->getRenderController('aggregator_feed')
    ...
    +    $items = $this->entityManager()->getStorageController('aggregator_item')->loadByFeed($aggregator_feed->id());
    
    @@ -232,25 +260,23 @@ public function adminOverview() {
    +        $items = $this->entityManager()->getStorageController('aggregator_item')->loadByCategory($category->cid);
    ...
    +          $summary_items = $this->entityManager()->getRenderController('aggregator_item')->viewMultiple($items, 'summary');
    
    @@ -295,24 +315,20 @@ public function sources() {
    +        $items = $this->entityManager()->getStorageController('aggregator_item')->loadByFeed($feed->id());
    ...
    +          $summary_items = $this->entityManager()
    

    lets store $this->entityManager() result to a variable. No need to always call the method

disasm’s picture

Spoke with @ParisLiakos in IRC. Removing __construct and create function, and adding two getters for the connection and categoryStorage variables. Creating variables for any getter methods called more than once in a function.

disasm’s picture

FileSize
9.57 KB

wrong interdiff, here's the right one.

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.

Status: Needs review » Needs work

The last submitted patch, drupal8.aggregator-module.1974408-75.patch, failed testing.

ParisLiakos’s picture

+++ w/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
@@ -8,26 +8,23 @@
-  protected $database;

@@ -37,42 +34,19 @@ class AggregatorController extends ControllerBase implements ContainerInjectionI
   protected $categoryStorage;

@@ -343,4 +323,29 @@ public function sources() {
+    if (!empty($this->connection)) {
+      $this->connection = $this->container->get('database');
+    }
+  }
...
+    if (!empty($this->categoryStorage)) {
+      $this->categoryStorage = $this->container->get('aggregator.category.storage');
+    }
+    return $this->categoryStorage;

i think we can easily remove those properties too and just do a one liner like ControlerBase currently does.

eg

protected function getDatabase() {
return $this->container->get('database');
}

ParisLiakos’s picture

so #2077513: Refactor ControllerBase to be more consistent with FormBase will fundamentaly change the way ControllerBase works, so i guess we should inject everything.
sorry for that

disasm’s picture

Status: Needs work » Needs review
FileSize
7.13 KB
24.82 KB

reverting #74 aside from #3 per discussion in IRC.

Status: Needs review » Needs work
Issue tags: -#pnx-sprint, -WSCCI-conversion, -RTBC July 1

The last submitted patch, drupal8.aggregator-module.1974408-80.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal8.aggregator-module.1974408-80.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
Issue tags: +#pnx-sprint, +WSCCI-conversion, +RTBC July 1
ParisLiakos’s picture

Status: Needs review » Needs work
+++ w/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
@@ -7,78 +7,46 @@
-    $this->database = $database;
...
+    $this->connection = $connection;

this is unnecessary change and also i think we use to call it database anyway, across core

besides that its ready to go

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
3.99 KB
23.84 KB

Reverts the rename of database => connection.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thanks

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Sorry to mark "needs work" for something so simple, but...

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
@@ -147,29 +176,29 @@ public function feedRefresh(FeedInterface $aggregator_feed, Request $request) {
-    $header = array(t('Title'), t('Items'), t('Last update'), t('Next update'), t('Operations'));
+    $header = array(t('Title'), $this->t('Items'), $this->t('Last update'), $this->t('Next update'), $this->t('Operations'));

@@ -182,16 +211,16 @@ public function adminOverview() {
-    $header = array(t('Title'), t('Items'), t('Operations'));
+    $header = array(t('Title'), $this->t('Items'), $this->t('Operations'));

Both of these are missing $this-> before t('Title'). I'd just fix it on commit but don't want to inadvertently make a typo that causes HEAD to fail, so testbot validation would be great.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
1.73 KB
23.85 KB

Fixes missing t() => $this->t() conversion.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Restoring RTBC. If the bot disagrees it'll move it back to needs work. :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great, thank you!

Committed and pushed to 8.x.

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

Anonymous’s picture

Issue summary: View changes

blocked