#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
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
Commit
None.
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 )
| 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 commentednice thought!
can we maybe name it just getDuplicates() then?
Comment #2
chx commentedComment #3
chx commentedComment #4
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 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 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 commentedLooks good
Comment #10
marcingy commentedI will look to reroll this not surprised it didn't pass!
Comment #11
marcingy commentedReroll and also killing dead use statements according too phpstorm that is anyway!
Comment #12
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 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 commentedthere
Comment #18
ParisLiakos commentedlol, wrong method
Comment #19
chx commentedLet's hope the bot agrees.
Comment #20
ParisLiakos commentedthanks
Comment #21
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 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.