Add contextual link to edit feed to aggregator blocks

Please find a patch creating the aggregator.links.contextual.yml and implementing the hook_block_view_BASE_BLOCK_ID_alter() in aggregator.module.

I don't know why SimpleTest failed here. It's working properly on my local and I can't see any test which should be updated to the new feature.

Update the patch to use the appropriate class.

Shouldn't this be in 'needs review'?

You are right...

1. +++ b/core/modules/aggregator/aggregator.module
   @@ -177,3 +178,17 @@ function aggregator_preprocess_block(&$variables) {
   +/**
   + * Implements hook_block_view_BASE_BLOCK_ID_alter() for 'aggregator_feed_block'.
   + */
   

   Should document the arguments here.

2. +++ b/core/modules/aggregator/aggregator.module
   @@ -177,3 +178,17 @@ function aggregator_preprocess_block(&$variables) {
   +  if (!empty($configuration['feed']) && isset($feeds[$configuration['feed']])) {
   

   Any reason why you want to do empty and isset? I'd do only empty if you really want a value or isset if you just want to know if the key was set. I'd do only isset here as you only pass it to the route_parameters and you don't care of the content.

Please find an updated patch and interdiff. I kept only the isset() but didn't documented arguments of the function as it's a hook.

Looks good, we still need tests for this though.

Added test

+++ b/core/modules/aggregator/src/Tests/AggregatorRenderingTest.php
@@ -27,6 +28,10 @@ class AggregatorRenderingTest extends AggregatorTestBase {
+    $this->container->get('module_installer')->install(['contextual']);

why not add it to the static $modules property?

can we also have a test only patch that fails?

why not add it to the static $modules property?

I didn't think it was relevant for the other test, should I just added to the modules property?

Will add the test only patch tonight.

btw this also needs a beta evaluation, this might be 8.1.x material

Removed the module install from the test and added it to the $modules property.
I also added the beta evaluation to the issue summary.

yes, we can file this under prioritized changes due to the usabilty impact

I tested this. The link is available after I clicked the contextual link. See screenshot.

+++ b/core/modules/aggregator/src/Tests/AggregatorRenderingTest.php
@@ -65,6 +67,19 @@ public function testBlockLinks() {
+    $this->assertRaw('<div data-contextual-id="'. $id . '"></div>', format_string('Contextual link placeholder with id @id exists.', array('@id' => $id)));

We shouldn't use format_string() here, it pollutes the safe string list. See #2549805: Remove all usage of FormattableMarkup in tests apart from explicit tests of that API

Good point

Thanks!
This looks fine to me now.

Setting back to RTBC as per #25

Sorry guys, this is not quite ready yet!

1. FeedViewBuilder should be updated too, similarly to NodeViewBuilder::alterBuild(), TermViewBuilder::alterBuild() etc.

2. +++ b/core/modules/aggregator/aggregator.links.contextual.yml
   @@ -0,0 +1,4 @@
   +  title: 'Configure feed'
   

   s/Configure/Edit/

   AFAIK we use "Configure" for config entities only.

3. +++ b/core/modules/aggregator/aggregator.module
   @@ -177,3 +178,17 @@ function aggregator_preprocess_block(&$variables) {
   +function aggregator_block_view_aggregator_feed_block_alter(array &$build, BlockPluginInterface $block) {
   

   This doesn't do the same additional check that menu_ui_block_view_system_menu_block_alter() does. Why not?

4. +++ b/core/modules/aggregator/aggregator.module
   @@ -177,3 +178,17 @@ function aggregator_preprocess_block(&$variables) {
   +  $feeds = \Drupal::entityManager()->getStorage('aggregator_feed')->loadMultiple();
   ...
   +  if (isset($feeds[$configuration['feed']])) {
   

   So we load all feeds just to check if it's an actual feed? Is this really necessary? This is very bad for performance. Let's at least only load the one we actually need.

   Also, this means that the edge case of the feed not existing is not tested.

5. +++ b/core/modules/aggregator/aggregator.module
   @@ -177,3 +178,17 @@ function aggregator_preprocess_block(&$variables) {
   +    $build['#contextual_links']['aggregator'] = array(
   +      'route_parameters' => array('aggregator_feed' => $configuration['feed']),
   +    );
   

   array() -> []

Working on it now.

1. Added the function alterBuild() to the FeedViewBuilder class.
2. Done.
3. This might be due to the implementation of hook_block_view_BASE_BLOCK_ID_alter. IMO
   

   if ($block->getBaseId() == 'system_menu_block') {
   

   should be removed from menu_ui_block_view_system_menu_block_alter (in a separate issue). Checking for BLOCK ID in a hook which has it in the function name does not make sense to me.
   

4. I was not able to find a way to load only one feed to check the validity. Can you please give me a pointer and I will accommodate it.
5. Done.

1. +++ b/core/modules/aggregator/aggregator.module
   @@ -177,3 +178,17 @@ function aggregator_preprocess_block(&$variables) {
   +  $feeds = \Drupal::entityManager()->getStorage('aggregator_feed')->loadMultiple();
   +
   +  $configuration = $block->getConfiguration();
   +  if (isset($feeds[$configuration['feed']])) {
   +    $build['#contextual_links']['aggregator'] = array(
   +      'route_parameters' => ['aggregator_feed' => $configuration['feed']],
   +    );
   +  }
   

   Can't we just change this to

   $build['#contextual_links']['aggregator'] = array(
     'route_parameters' => ['aggregator_feed' => $configuration['feed']],
   );
   

   Since the feed id should always be an existing feed anyway. A block will only be build if it is a valid feed id anyway.

2. +++ b/core/modules/aggregator/src/Tests/AggregatorRenderingTest.php
   @@ -65,6 +67,19 @@ public function testBlockLinks() {
   +    $this->assertIdentical($json[$id], '<ul class="contextual-links"><li class="block-configure"><a href="' . base_path() . 'admin/structure/block/manage/' . $block->id() . '">Configure block</a></li><li class="aggregator-feed-edit"><a href="' . base_path() . 'aggregator/sources/' . $feed->id() . '/configure">Configure feed</a></li></ul>');
   

   We stil need to change this to 'Edit feed' also.

For the test fails, i cant't check right now but i think you still have to add this to AggregatorFeedBlock.php

use Drupal\Core\Entity\Display\EntityViewDisplayInterface;

I think this should be green.

That last patch was not complete.

Getting late, ignore that last interdiff also :)

Almost there now! :)

1. +++ b/core/modules/aggregator/aggregator.links.contextual.yml
   @@ -0,0 +1,4 @@
   +entity.aggregator_feed.edit:
   

   I see this now has an entity prefix. Node also does this, so yay for consistency!

2. +++ b/core/modules/aggregator/aggregator.links.contextual.yml
   @@ -0,0 +1,4 @@
   +  group: aggregator
   

   This should be aggregator_feed, because aggregator provides two entity types: Feed & Item. In theory, we may add contextual links to Item entities in the future, so we should be sufficiently specific here.

   (Yes, this "group" concept for contextual links is totally WTF. But that's out of scope here.)

3. +++ b/core/modules/aggregator/aggregator.module
   @@ -177,3 +178,13 @@ function aggregator_preprocess_block(&$variables) {
   +function aggregator_block_view_aggregator_feed_block_alter(array &$build, BlockPluginInterface $block) {
   

   So much better!

4. +++ b/core/modules/aggregator/aggregator.module
   @@ -177,3 +178,13 @@ function aggregator_preprocess_block(&$variables) {
   +  $build['#contextual_links']['aggregator'] = array(
   

   Still array().

5. +++ b/core/modules/aggregator/src/FeedViewBuilder.php
   @@ -132,8 +134,19 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +    $build['#contextual_links']['aggregator'] = array(
   

   Also array().

6. +++ b/core/modules/aggregator/src/FeedViewBuilder.php
   @@ -132,8 +134,19 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +      'metadata' => ['changed' => $entity->getLastModified()],
   

   Ugh, this is called getLastModified() instead of getChangedTime() because FeedInterface does not extend EntityChangedInterface. Could you file an issue for that and add a @todo?

1. Was thinking the same thing :)
2. Done
3. Yes, that would have been a bit expensive
4. Oops missed those yesterday
5. same
6. Done #2554079: FeedInterface should extend EntityChangedInterface

reroll

@ParisLiakos remarked in #2554079-3: FeedInterface should extend EntityChangedInterface that Feed::getLastModifiedTime() is about the external feed's last modification timestamp.

The patch in #46 that has

'metadata' => ['changed' => $entity->getLastModified()],

tricked me into thinking it was the feed entity's own last modification time!

1. +++ b/core/modules/aggregator/src/FeedInterface.php
   @@ -200,6 +200,9 @@ public function setEtag($etag);
   +   * @todo Remove this function and extend from EntityChangedInterface instead
   +   *   https://www.drupal.org/node/2554079.
   

   Therefor this…

2. +++ b/core/modules/aggregator/src/FeedViewBuilder.php
   @@ -133,8 +135,19 @@ public function buildComponents(array &$build, array $entities, array $displays,
   +      'metadata' => ['changed' => $entity->getLastModified()],
   

   … and this should simply be deleted.

Sorry for having missed that!

Yup, that makes sense.

What'd be helpful for a committer is if this also had a screenshot showing the contextual link on both a feed block and a feed page.

Once we have that, this is RTBC :)

Here you go.

Feed block:

Feed page:

And thanks for reviewing, that was quite helpful.

#54: You're welcome :) Thanks for helping make D8 better!

Note that this may end up being postponed to 8.1 because it's a feature request. The only reason I've not moved it to 8.1 is because it's so tiny.

#56 was probably a random fail so setting this back to RTBC

random fail, back to RTBC

Rerolling.

$ sudo git rebase 8.0.x
First, rewinding head to replay your work on top of it...
Applying: Patch applied.
Using index info to reconstruct a base tree...
M	core/modules/aggregator/aggregator.module
M	core/modules/aggregator/src/FeedViewBuilder.php
M	core/modules/aggregator/src/Tests/AggregatorRenderingTest.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/aggregator/src/Tests/AggregatorRenderingTest.php
Auto-merging core/modules/aggregator/src/FeedViewBuilder.php
Auto-merging core/modules/aggregator/aggregator.module

Missed out a file from the patch above. Added it back.

Was a plain reroll. Setting back to RTBC.

Thanks for working on this everyone. Unfortunately this is not eligible for the Drupal 8.0 release as it adds risks of breaking something. This is a feature and is not required to be part of Drupal 8 for release - we should target this for release in a minor version.

#68++

I already said something similar in #55:

Note that this may end up being postponed to 8.1 because it's a feature request. The only reason I've not moved it to 8.1 is because it's so tiny.

Good to have this ready though :)

The aggregator module has been removed from Core in 10.0.x-dev and now lives on as a contrib module. Issues in the Core queue about the aggregator module, like this one, have been moved to the contrib module queue.