Everything that typehints Feed should typehint FeedInterface instead

CommentFileSizeAuthor
#7 drupal-aggregator-feedinterface-typehints-2191273-7.patch18.19 KBmfernea
#6 drupal-aggregator-feedinterface-typehints-2191273-6.patch17.04 KBmfernea
#4 drupal-aggregator-feedinterface-typehints-2191273-4.patch18.17 KBmfernea
#2 drupal-aggregator-feedinterface-typehints-2191273-2.patch18.15 KBmfernea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mfernea’s picture

mfernea CreditAttribution: mfernea commented
Assigned: Unassigned » mfernea

I'll work on this and post a patch later on.

mfernea’s picture

mfernea CreditAttribution: mfernea commented
Status: Active » Needs review
FileSize
drupal-aggregator-feedinterface-typehints-2191273-2.patch18.15 KB

Here is the patch.

I don't think that "EntityInterface" inside "template_preprocess_aggregator_summary_items" should be replaced, should it?
$variables['title'] = check_plain($variables['source'] instanceof EntityInterface ? $variables['source']->label() : $variables['source']->title);
and
$variables['source_url'] = $variables['source'] instanceof EntityInterface ? $variables['source']->url->value : $variables['source']->url;

Status: Needs review » Needs work

The last submitted patch, 2: drupal-aggregator-feedinterface-typehints-2191273-2.patch, failed testing.

mfernea’s picture

mfernea CreditAttribution: mfernea commented
Status: Needs work » Needs review
FileSize
drupal-aggregator-feedinterface-typehints-2191273-4.patch18.17 KB

It needed a reroll.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented

thanks for the patch!
the instanceof EntityInterface is a temp hack and we should be able to remove it in #1957312: Use the entity storage controller in aggregator module
so you can leave it as is.
i think your patch needs one more reroll unfortunately due to #2138115: Split aggregator theme functions to a separate file

mfernea’s picture

mfernea CreditAttribution: mfernea commented
FileSize
drupal-aggregator-feedinterface-typehints-2191273-6.patch17.04 KB

Ok. Here is the new patch.

mfernea’s picture

mfernea CreditAttribution: mfernea commented
FileSize
drupal-aggregator-feedinterface-typehints-2191273-7.patch18.19 KB

With the new modifications I missed 2 usages of Feed. I fixed that.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs review » Reviewed & tested by the community

thank you!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Component: aggregator.module » documentation

Moving to the documentation queue for @jhodgdon.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Component: documentation » aggregator.module

This is not all documentation. There are use statements being changed in several places. I'll let one of the other committers take this one.

mfernea’s picture

mfernea CreditAttribution: mfernea commented

I looked over all change records that mentioned Feed and I found those two in which I modified the typehinting:
https://drupal.org/node/2078169
https://drupal.org/node/1957310

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 2d65e60 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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