Right now aggregator has 2 path for accomplishing the very same thing.

admin/config/services/aggregator/edit/feed/%aggregator_feed
and
aggregator/sources/%aggregator_feed/configure
Lets merge those in the aggregator/sources one

Also the delete
admin/config/services/aggregator/delete/feed/%aggregator_feed
and add path
admin/config/services/aggregator/add/feed
Should be moved to aggregator/sources one so we are consistent with what we do in other places in core, eg node or block

CommentFileSizeAuthor
#12 drupal-aggregator_routes_cleanup-2152673-12.patch11.19 KBParisLiakos
#12 aggreagator-routes.interdiff.txt1.56 KBParisLiakos
#10 drupal-aggregator_routes_cleanup-2152673-10.patch11.51 KBParisLiakos
#9 interdiff.txt583 bytesParisLiakos
#9 drupal-aggregator_routes_cleanup-2152673-9.patch13.31 KBParisLiakos
#5 drupal-aggregator_routes_cleanup-2152673-5.patch13.11 KBParisLiakos
#3 drupal-aggregator_routes_cleanup-2152673-3.patch12.14 KBParisLiakos
#1 drupal-aggregator_routes_cleanup-2152673-1.patch6.57 KBParisLiakos

Comments

ParisLiakos’s picture

ParisLiakos commented
StatusFileSize
new drupal-aggregator_routes_cleanup-2152673-1.patch6.57 KB
ParisLiakos’s picture

ParisLiakos commented
Status: Active » Needs review
ParisLiakos’s picture

ParisLiakos commented
Title: Remove duplicate edit route from aggregator and make delete consistent with rest of core » Remove duplicate edit route from aggregator and make delete/add consistent with rest of core
Issue summary: View changes
StatusFileSize
new drupal-aggregator_routes_cleanup-2152673-3.patch12.14 KB

Status: Needs review » Needs work

The last submitted patch, 3: drupal-aggregator_routes_cleanup-2152673-3.patch, failed testing.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review
StatusFileSize
new drupal-aggregator_routes_cleanup-2152673-5.patch13.11 KB

The last submitted patch, 1: drupal-aggregator_routes_cleanup-2152673-1.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

I would always consider aggregator to be part of the admin interface to be honest.

I installed it via simplytest and found /aggregator/sources and /aggregator clickable, but none of them had a link to add a new entry. Should we maybe add some menu link/local action as well?
From the pure code side, there is nothing to say.

ParisLiakos’s picture

ParisLiakos commented

The add local action is in
admin/config/services/aggregator

/aggregator/sources is similar like the /node path. There is no add action there. Actually now that i think of, do we have local actions in frontend at all? But yes as a menu link it is actually a good idea. Like the "Add content" one. i ll roll a patch for that

I just think that feeds and its operations should have /aggregator/sources as base path like we do for other entities already in core. The sake of consistency

ParisLiakos’s picture

ParisLiakos commented
StatusFileSize
new drupal-aggregator_routes_cleanup-2152673-9.patch13.31 KB
new interdiff.txt583 bytes
ParisLiakos’s picture

ParisLiakos commented
StatusFileSize
new drupal-aggregator_routes_cleanup-2152673-10.patch11.51 KB

reroll

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work 
+++ b/core/modules/aggregator/aggregator.module
@@ -127,6 +131,18 @@ function aggregator_permission() {
+ * Implements hook_admin_paths().
+ */
+function aggregator_admin_paths() {
+  $paths = array(
+    'aggregator/sources/add' => TRUE,
+    'aggregator/sources/*/configure' => TRUE,
+    'aggregator/sources/*/delete' => TRUE,
+  );
+  return $paths;
+}
+

We have a new system for that now.

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review
StatusFileSize
new aggreagator-routes.interdiff.txt1.56 KB
new drupal-aggregator_routes_cleanup-2152673-12.patch11.19 KB

oh yes, which is awesome:)

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

These changes are perfect!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Very nice clean-up.

Committed and pushed to 8.x. Thanks!

  • Commit 7d7b24f on 8.x by webchick: 
    Issue #2152673 by ParisLiakos: Remove duplicate edit route from...
ParisLiakos’s picture

ParisLiakos commented

i messed up the yaml definition, opened #2224951: Aggregator delete local task uses tab_root_id instead of base_route with correction + test

Status: Fixed » Closed (fixed)

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