Files: 
CommentFileSizeAuthor
#90 1974408-aggregator-source-controller-90.patch23.85 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 59,067 pass(es).
[ View ]
#90 interdiff.txt1.73 KBkim.pepper
#87 1974408-aggregator-source-controller-87.patch23.84 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 59,652 pass(es).
[ View ]
#87 interdiff.txt3.99 KBkim.pepper
#81 drupal8.aggregator-module.1974408-80.patch24.82 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,711 pass(es).
[ View ]
#81 interdiff.txt7.13 KBdisasm
#76 interdiff.txt9.57 KBdisasm
#75 drupal8.aggregator-module.1974408-75.patch24.97 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,920 pass(es), 120 fail(s), and 46 exception(s).
[ View ]
#75 interdiff.txt958 bytesdisasm
#73 1974408-aggregator-source-controller-73.patch24.55 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,933 pass(es).
[ View ]
#73 interdiff.txt958 byteskim.pepper
#72 drupal8.aggregator-module.1974408-72.patch24.65 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,903 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#72 interdiff.txt5.36 KBdisasm
#69 1974408-aggregator-source-controller-69.patch23.76 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 58,937 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#69 interdiff.txt812 byteskim.pepper
#67 drupal8.aggregator-module.1974408-67.patch23.83 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,911 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#67 interdiff.txt17.96 KBdisasm
#65 drupal8.aggregator-module.1974408-65.patch19.96 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#65 interdiff.txt14.09 KBdisasm
#62 1974408-aggregator-source-controller-62.patch13.88 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 58,446 pass(es), 66 fail(s), and 219 exception(s).
[ View ]
#62 interdiff.txt10.51 KBkim.pepper
#54 1974408-aggregator-source-controller-54.patch11.55 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 57,218 pass(es).
[ View ]
#54 interdiff.txt904 bytesParisLiakos
#52 1974408-aggregator-source-controller-52.patch11.45 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,562 pass(es).
[ View ]
#52 interdiff.txt3.4 KBParisLiakos
#49 1974408-aggregator-source-controller-49.patch12.07 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 56,710 pass(es), 29 fail(s), and 26 exception(s).
[ View ]
#49 interdiff.txt920 bytesParisLiakos
#46 1974408-aggregator-source-controller-46.patch12.21 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,605 pass(es).
[ View ]
#46 interdiff.txt6.99 KBParisLiakos
#45 1974408-aggregator-source-controller-45.patch8.13 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 56,390 pass(es).
[ View ]
#45 interdiff.txt632 byteskim.pepper
#43 1974408-aggregator-source-controller-42.patch8.11 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,042 pass(es).
[ View ]
#43 interdiff.txt1.63 KBkim.pepper
#38 1974408-aggregator-source-controller-38.patch8.15 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 56,682 pass(es), 8 fail(s), and 230 exception(s).
[ View ]
#38 interdiff.txt1.6 KBkim.pepper
#33 1974408-aggregator-source-controller-33.patch8.14 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,025 pass(es).
[ View ]
#33 interdiff.txt4.54 KBkim.pepper
#29 1974408-aggregator-source-controller-29.patch7.82 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,467 pass(es).
[ View ]
#29 interdiff.txt6.75 KBkim.pepper
#26 1974408-aggregator-source-controller-26.patch6.71 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 58,014 pass(es), 29 fail(s), and 0 exception(s).
[ View ]
#26 interdiff.txt755 byteskim.pepper
#24 aggregator-page-source-controller-1974408-24.patch6.71 KBpwieck
FAILED: [[SimpleTest]]: [MySQL] 56,739 pass(es), 29 fail(s), and 0 exception(s).
[ View ]
#18 aggregator-page-source-controller-1974408-18.patch7.16 KBtheMusician
PASSED: [[SimpleTest]]: [MySQL] 55,891 pass(es).
[ View ]
#11 aggregator-page-source-controller-1974408-11.patch7.65 KBkshama_deshmukh
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch aggregator-page-source-controller-1974408-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 2-9-interdiff.txt1.77 KBalexpott
#9 aggregator-page-source-controller-1974408-9.patch4.88 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 55,372 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#2 aggregator-page-source-controller-1974408-2.patch3.38 KBkshama_deshmukh
FAILED: [[SimpleTest]]: [MySQL] 55,406 pass(es), 36 fail(s), and 27 exception(s).
[ View ]

Comments

Assigned:Unassigned» kshama_deshmukh

StatusFileSize
new3.38 KB
FAILED: [[SimpleTest]]: [MySQL] 55,406 pass(es), 36 fail(s), and 27 exception(s).
[ View ]

Status:Active» Needs review

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.

Issue tags:+LONDON_2013_APRIL

Tagging

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 :)

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new4.88 KB
FAILED: [[SimpleTest]]: [MySQL] 55,372 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
new1.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.

Status:Needs work» Needs review
StatusFileSize
new7.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch aggregator-page-source-controller-1974408-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

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

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.

Issue tags:+Needs reroll

Working on re-rolling this right now.

StatusFileSize
new7.16 KB
PASSED: [[SimpleTest]]: [MySQL] 55,891 pass(es).
[ View ]

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.

Status:Needs work» Needs review

Ugh...forgot to set for review.

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.

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

Issue tags:-Needs reroll

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

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new6.71 KB
FAILED: [[SimpleTest]]: [MySQL] 56,739 pass(es), 29 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new755 bytes
new6.71 KB
FAILED: [[SimpleTest]]: [MySQL] 58,014 pass(es), 29 fail(s), and 0 exception(s).
[ View ]

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

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

Status:Needs work» Needs review
StatusFileSize
new6.75 KB
new7.82 KB
PASSED: [[SimpleTest]]: [MySQL] 58,467 pass(es).
[ View ]

Thanks for the review @dawehner.

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

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

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

Issue tags:-Stalking Crell, -LONDON_2013_APRIL
StatusFileSize
new4.54 KB
new8.14 KB
PASSED: [[SimpleTest]]: [MySQL] 58,025 pass(es).
[ View ]

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.

Issue tags:+#pnx-sprint

Tagging

about the pager thing:
it should be

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

instead of

<?php
$build
['pager']['#markup'] = theme('pager');
?>

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

StatusFileSize
new1.6 KB
new8.15 KB
FAILED: [[SimpleTest]]: [MySQL] 56,682 pass(es), 8 fail(s), and 230 exception(s).
[ View ]

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.

Looks like I messed up the render array stuff.

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

Is this right?

Double-post

Status:Needs work» Needs review
Issue tags:-#pnx-sprint, -WSCCI-conversion
StatusFileSize
new1.63 KB
new8.11 KB
PASSED: [[SimpleTest]]: [MySQL] 58,042 pass(es).
[ View ]

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

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

Status:Needs work» Needs review
StatusFileSize
new632 bytes
new8.13 KB
PASSED: [[SimpleTest]]: [MySQL] 56,390 pass(es).
[ View ]

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

StatusFileSize
new6.99 KB
new12.21 KB
PASSED: [[SimpleTest]]: [MySQL] 56,605 pass(es).
[ View ]

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:)

Status:Needs review» Reviewed & tested by the community

Looks good to me!

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

StatusFileSize
new920 bytes
new12.07 KB
FAILED: [[SimpleTest]]: [MySQL] 56,710 pass(es), 29 fail(s), and 26 exception(s).
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new3.4 KB
new11.45 KB
PASSED: [[SimpleTest]]: [MySQL] 56,562 pass(es).
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new904 bytes
new11.55 KB
PASSED: [[SimpleTest]]: [MySQL] 57,218 pass(es).
[ View ]

added url generator docs

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

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

Assigned:Unassigned» kim.pepper

@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

Issue summary:View changes

Link to meta issue

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.

Issue summary:View changes

related issues

Status:Postponed» Active

unblocked

Status:Active» Needs review
StatusFileSize
new10.51 KB
new13.88 KB
FAILED: [[SimpleTest]]: [MySQL] 58,446 pass(es), 66 fail(s), and 219 exception(s).
[ View ]
  • 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

  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.

StatusFileSize
new14.09 KB
new19.96 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

Status:Needs work» Needs review

StatusFileSize
new17.96 KB
new23.83 KB
FAILED: [[SimpleTest]]: [MySQL] 58,911 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

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.

StatusFileSize
new812 bytes
new23.76 KB
FAILED: [[SimpleTest]]: [MySQL] 58,937 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Fixes an issue with $this->redirect()

Status:Needs work» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new5.36 KB
new24.65 KB
FAILED: [[SimpleTest]]: [MySQL] 58,903 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

StatusFileSize
new958 bytes
new24.55 KB
PASSED: [[SimpleTest]]: [MySQL] 58,933 pass(es).
[ View ]

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.

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

StatusFileSize
new958 bytes
new24.97 KB
FAILED: [[SimpleTest]]: [MySQL] 58,920 pass(es), 120 fail(s), and 46 exception(s).
[ View ]

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.

StatusFileSize
new9.57 KB

wrong interdiff, here's the right one.

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.

+++ 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');
}

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

Status:Needs work» Needs review
StatusFileSize
new7.13 KB
new24.82 KB
PASSED: [[SimpleTest]]: [MySQL] 58,711 pass(es).
[ View ]

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

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

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

Status:Needs work» Needs review
StatusFileSize
new3.99 KB
new23.84 KB
PASSED: [[SimpleTest]]: [MySQL] 59,652 pass(es).
[ View ]

Reverts the rename of database => connection.

Status:Needs review» Reviewed & tested by the community

thanks

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.

Status:Needs work» Needs review
StatusFileSize
new1.73 KB
new23.85 KB
PASSED: [[SimpleTest]]: [MySQL] 59,067 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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

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.

Issue summary:View changes

blocked