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.
Followup from #1930274-45: Convert aggregator processors and parsers to plugins
Replace db_* calls for aggregator feeds and items with the entity storage controller/ entity_query equilavent
Comment | File | Size | Author |
---|---|---|---|
#67 | 1957312-drupal-aggregator_entity_storage-67.patch | 28.39 KB | ParisLiakos |
#65 | 1957312-drupal-aggregator_entity_storage-65.patch | 28.37 KB | ParisLiakos |
#58 | interdiff.txt | 3.58 KB | ParisLiakos |
#58 | 1957312-drupal-aggregator_entity_storage-58.patch | 26.66 KB | ParisLiakos |
#57 | 1957312-drupal-aggregator_entity_storage-57.patch | 26.63 KB | ParisLiakos |
Comments
Comment #1
ParisLiakos CreditAttribution: ParisLiakos commentedi have no clue what to do with
The other change in aggregator_cron() could use some benchmarking?
see #1930274-53: Convert aggregator processors and parsers to plugins
Comment #3
ParisLiakos CreditAttribution: ParisLiakos commentedoops
Comment #4
ParisLiakos CreditAttribution: ParisLiakos commentedsigh..status
Comment #6
ParisLiakos CreditAttribution: ParisLiakos commentedEntityFilteringThemeTest.php line 122
random failure?
#3: drupal-aggregator_entity_storage-1957312-3.patch queued for re-testing.
Comment #7
twistor CreditAttribution: twistor commentedLooks good so far.
This should still use entity_delete_multiple().
entity_delete_multiple().
Comment #8
ParisLiakos CreditAttribution: ParisLiakos commentedentity_delete_multiple() needs the id array..so i looped the items instead...hmm but now that you say so, i think entity_load_multiple() returns the array keyed by ids so i could use array_keys i think, will check later
Comment #9
twistor CreditAttribution: twistor commentedIt takes the same arguments as entity_load_multiple().
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedthis is a lot better, you are right:)
Comment #11
andypostThis needs reroll - #1957148: Replace entity_query() with Drupal::entityQuery()
Comment #12
twistor CreditAttribution: twistor commentedThere's 2 ways we can fix the hook_cron() query.
We could change the last column to next. Most things that last was needed for could be changed to next - refresh, but then we wouldn't be accurately reporting the last run time if the refresh time changed.
Comment #13
plachAdded this to #2068325: [META] Convert entity SQL queries to the Entity Query API.
Comment #14
Désiré CreditAttribution: Désiré commentedComment #15
Désiré CreditAttribution: Désiré commentedNow I'm just rerolled the patch, without the core/modules/aggregator/lib/Drupal/aggregator/Plugin/Derivative/AggregatorFeedBlock.php modifications, since it was removed.
But there are other new SQL queries in the code which should be changed too.
Put it to needs review only for testing the rerolled patch.
Comment #16
Désiré CreditAttribution: Désiré commentedComment #17
ParisLiakos CreditAttribution: ParisLiakos commentedwe can now inject stuff to plugins so entity query should be injected.
and the item storage controller
There is also one more query in \Drupal\aggregator\Plugin\Block\AggregatorFeedBlock::build() to get rid of and one in \Drupal\aggregator\Controller\AggregatorController::adminOverview()
Comment #18
Désiré CreditAttribution: Désiré commentedComment #19
Désiré CreditAttribution: Désiré commentedThe two remaining query are replaced.
I think the feeds category query could be not replaced since the feeds categories aren't entities.
I'm a kind of new with D8 so can you help me, how can I do these things with dependency injection?
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedyou dont have to use the entityQuery at all. i think just calling entity_load_mutiple without the second parameter, gives you all the entities
this method should be moved to the FeedStorageController instead (and also added to its interface)
to inject this:
in the class::create() method, add as parameter $container->get('entity.query')->get('aggregator_item') to the new static() call.
This will pass it to the class constructor as additional argument, where you will take it and store it to a property. eg $this->itemEntityQuery.
then instead of using \Drupal::entityQuery('aggregator_item') use the property instead;)
same for this but in the create method you will add
$container->get('entity.manager')->getStorageController('aggregator_item')
we should add the ORDER BY part in the entity query too
Comment #22
plachAre you still working on this? Otherwise tomorrow we should be able to bring it forward at the Prague extended sprint.
Comment #23
plachComment #24
peximo CreditAttribution: peximo commentedOk I work on this.
Comment #25
peximo CreditAttribution: peximo commentedOk, modified as suggested.
Some considerations:
1. aggregator_save_category() seems never called: we need to remove this function? Or we need to convert it anyway?
2. aggregator_page_opml() is deprecated, we need to convert it anyway?
3. [12] @chx has suggested to first move the logic in the storage controller and than open a follow up issue.
Comment #26
peximo CreditAttribution: peximo commentedSorry wrong status.
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commentedlets retrieve the storage controller before the entity_load_multiple call, and use it there too.
also it should be retrieved with $this->entityManager()
should be typehinted with FeedInterface
so this actually retrieves the feed IDs not the whole feed objects.
So i would say, name it getFeedIdsToRefresh().
Also it shouldnt return the query, but rather it should fetchCol() first
i think this should be feedStroage not itemStorage
this should also use the itemStorage to load and not the procedural function
lets inject the entity_query and the item storage here as described in #21
Comment #29
peximo CreditAttribution: peximo commentedRerolled and modified as suggested, needs work because some tests fail.
Comment #30.0
(not verified) CreditAttribution: commentedUpdated issue summary
Comment #31
plachComment #32
peximo CreditAttribution: peximo commentedRerolled with some adjustment.
Comment #34
peximo CreditAttribution: peximo commentedRerolled; the work should be finished.
Comment #37
plach34: 1957312-drupal-aggregator_entity_storage-34.patch queued for re-testing.
Comment #38
ParisLiakos CreditAttribution: ParisLiakos commentedlooks great to me, thanks!
Just one thing
we should use the loadByFeed() method in ItemStorageController..By making the method there to default to NULL $limit rather than 20.So one can use it to fetch all items too.. (Change that would affect loadByCategory() too)
and also a reroll
Comment #39
plach@peximo
Are you still working on this? Otherwise unassign it so that someone else can pick it up :)
Comment #40
plach@peximo:
Are you still working on this? Otherwise unassign it so that someone else can pick it up :)
Comment #41
Berdir34: 1957312-drupal-aggregator_entity_storage-34.patch queued for re-testing.
Comment #43
peximo CreditAttribution: peximo commented@plach:
I can work on it a bit in the coming weekend.
Comment #44
peximo CreditAttribution: peximo commentedRerolled, I have changed as suggested in #38.
@ParisLiakos: loadByCategory() is gone, we need to change also loadAll()?
Comment #46
ParisLiakos CreditAttribution: ParisLiakos commented@peximo Yes exactly:) thanks for rolling this
Comment #47
peximo CreditAttribution: peximo commentedRerolled with some adjustments; if green I hope the work is done.
Comment #48
ParisLiakos CreditAttribution: ParisLiakos commentedThanks! Looks almost done.
Just a few docs issues i could find
Probably feed here should be plural. ie feeds
it should default to NULL, not FALSE :)
ItemStorageControllerInterface::loadAll()
docblock still says it defaults to 20Comment #49
peximo CreditAttribution: peximo commentedOk, I removed also a non existent default limit in
executeFeedItemQuery()
docblock.Comment #50
peximo CreditAttribution: peximo commentedsorry I forgot the intediff.
Comment #51
ParisLiakos CreditAttribution: ParisLiakos commentedThank you!
Comment #52
catchCan't use entityQuery here?
Comment #53
ParisLiakos CreditAttribution: ParisLiakos commentedah, yes, good point. there is a count() method in QueryInterface.
also i found a couple forgotten spots that need to be converted as well
Comment #54
ParisLiakos CreditAttribution: ParisLiakos commentedyes, its a lot better now:)
getItemCount should be in the Item storage controller not the Feed one.
also got rid the database from aggregator controller
Comment #56
ParisLiakos CreditAttribution: ParisLiakos commentedah
Comment #57
ParisLiakos CreditAttribution: ParisLiakos commentedreroll
Comment #58
ParisLiakos CreditAttribution: ParisLiakos commentedlets also use the methods now that #2028037: Expand FeedInterface with methods is in.
this should be good to go now
Comment #59
ParisLiakos CreditAttribution: ParisLiakos commentedReroll plus some docs fixes/additions
Comment #60
jibran59: 1957312-drupal-aggregator_entity_storage-59.patch queued for re-testing.
Comment #62
ParisLiakos CreditAttribution: ParisLiakos commentedreroll
Comment #63
ParisLiakos CreditAttribution: ParisLiakos commentedand of course, we dont need that hack anymore:)
Comment #64
ParisLiakos CreditAttribution: ParisLiakos commentedone more reroll
Comment #65
ParisLiakos CreditAttribution: ParisLiakos commentedreroll
Comment #66
jibranThanks for sticking with it. I think we are done here.
Comment #67
ParisLiakos CreditAttribution: ParisLiakos commentedyay, thanks!
but aggregator goes faaaast those days:D
rerolls++
Comment #69
ParisLiakos CreditAttribution: ParisLiakos commented67: 1957312-drupal-aggregator_entity_storage-67.patch queued for re-testing.
Comment #70
ParisLiakos CreditAttribution: ParisLiakos commentedComment #71
webchickNice work!
Committed and pushed to 8.x. Thanks!
Comment #73
chx CreditAttribution: chx commentedI filed #2228733: Remove getFeedDuplicates - its unused and untested as a followup.