Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
#1957312: Use the entity storage controller in aggregator module converted getFeedDuplicates to entity query making it storage independent. #2403817: Feed entity validation misses form validation logic Removed usage
Proposed resolution
Remove it since its untested. Not only that, but it makes life harder for folks that write storage classes, cause it doesnt need to be there since it uses entity query
Remaining tasks
Commit
User interface changes
None.
API changes
getFeedDuplicates() is removed. (no change notice is needed because it has no D7 equilavent. the method was introduced in #2041083: Move database query out of \Drupal\aggregator\FeedFormController )
Beta phase evaluation
| Issue category | Task because it does not affect any functionality |
|---|---|
| Prioritized changes | Prioritized because it removes unused and untested code which reduces fragility. |
| Disruption | Although an interface method is removed, the method was introduced recently within the 8.x dev cycle and i the usages in contrib/custom code should be really low. |
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | interdiff.txt | 805 bytes | ParisLiakos |
| #18 | aggregator-feed_duplicates-2228733-18.patch | 1.75 KB | ParisLiakos |
| #16 | aggregator-feed_duplicates-2228733.patch | 1.57 KB | ParisLiakos |
| #11 | move-entity-feed.patch | 4.33 KB | marcingy |
| #2 | 2228733_2.patch | 4.38 KB | chx |
Comments
Comment #1
ParisLiakos CreditAttribution: ParisLiakos commentednice thought!
can we maybe name it just getDuplicates() then?
Comment #2
chx CreditAttribution: chx commentedComment #3
chx CreditAttribution: chx commentedComment #4
ParisLiakos CreditAttribution: ParisLiakos commentedI am sorry:/ i gave this a bit more thought:
Since this is only used in just one place and it is validation logic, it makes more sense to just move the whole entity query to the form.
I cant see this method being useful anywhere else outside this validation callback.
Comment #5
dawehnerI have to disagree with paris here. This is generic functionality, no matter how often core uses it.
On the other hand this also does not seem to make sense on the feed object. There seems to be room for a new service on which both getFeedIdsToRefresh and getFeedDuplicates exists.
Comment #6
chx CreditAttribution: chx commentedTwo notes:
a) I am out of this issue. It bordered MongoDB work (I needed copypaste this method) so I did it but the moment it's controversial, I am out.
b) ItemStorage has no database in it at all either.
Comment #7
chx CreditAttribution: chx commentedBecause plach asked what's the problem: well, the problem is that PHP is single inheritance and a class replacing the storage need to inherit from its own base class and then copy over these storage independent methods. In general, storage classes should contain the absolute minimum storage cos everything else will need to be copypasted.
Comment #8
marcingy CreditAttribution: marcingy commentedLooks good
Comment #10
marcingy CreditAttribution: marcingy commentedI will look to reroll this not surprised it didn't pass!
Comment #11
marcingy CreditAttribution: marcingy commentedReroll and also killing dead use statements according too phpstorm that is anyway!
Comment #12
slashrsm CreditAttribution: slashrsm commentedLooks good.
Comment #13
alexpottThese two comments suggest that there might be a better place for this. And that we need to consider how to implement general functionality for storage classes.
Comment #14
dawehnerMore like needs work tbh.
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commented#2403817: Feed entity validation misses form validation logic removed usage, so we can just remove it. its now unused and untested
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedthere
Comment #18
ParisLiakos CreditAttribution: ParisLiakos commentedlol, wrong method
Comment #19
chx CreditAttribution: chx commentedLet's hope the bot agrees.
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedthanks
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedComment #22
alexpottThis issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.
Removing unused and untested code reduces fragility so +1 to the change.
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedadded beta evaluation
Comment #24
alexpottLess code - nice. Committed 5951f55 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.