Updated: Comment #82
Problem/Motivation
Aggregator categories are one of the last core sub-systems that do not use the entity API.
Instead it relies on its own API. While we tried making its storage swappable with a seperate service, categories are still un-translatable and we are still re-inventing the wheel.
Proposed resolution
Categories should be Terms instead.
Get rid of the custom code and the raw-sql queries. Use the entity API instead.
Get rid of the custom tables that handle the relations between categories and feeds/feed items and instead use Field API ; Attach an entity reference field on feeds and feed items entities instead.
We will also need an aggregator specific vocabulary to put existing categories from sites upgrading from D7.
Remaining tasks
A maintainer is needed to approve this API change for D8
Reroll patch in #72 and see wherre we stand with tests after #2004316: Exception on field_purge_batch() after deleting a field was fixed.
Fix any potential newly introduced failures.
Review and RTBC
User interface changes
All the category crud forms will use the term forms instead provided by the taxonomy module, which means taxonomy will be a dependency
Aggregator will get its own field-ui for managing feed entities under the current administration page
API changes
aggregator_category
, aggregator_category_feed
and aggregator_category_item
tables are being removed in favor of taxonomy tables and tables created by Field API
aggregator_category_selector
is removed from aggregator's config and moves to the field instance settings instead
aggregator_save_category
is removed in favor of just $category->save()
aggregator_category_load
is removed in favor of entity_load
Original report by qgil
The Aggregator module allows the creation of categories to file the news feed. The creation of categories is done manually, though.
What about the possibility to assign an already created vocabulary to define the aggrgator categories?
It would be quite useful. For example, if my site has a vocabulary "topic" with entries such as film, radio, tv, press with which I organize articles and the forum, I could use this same vocabulary to organize news feeds.
Comment | File | Size | Author |
---|---|---|---|
#72 | drupal-aggregator_taxonomy-15266-72.patch | 104.29 KB | ParisLiakos |
#68 | drupal-aggregator_taxonomy-15266-68.patch | 106.08 KB | ParisLiakos |
#70 | drupal-aggregator_taxonomy-15266-70.patch | 105.19 KB | ParisLiakos |
#62 | drupal-aggregator_taxonomy-15266-62.patch | 102.33 KB | twistor |
#62 | interdiff.txt | 3.74 KB | twistor |
Comments
Comment #1
magico CreditAttribution: magico commentedAnything done about this?
Comment #2
LAsan CreditAttribution: LAsan commentedAny news about this?
Moving to cvs.
Comment #3
alex_b CreditAttribution: alex_b commented#293318: Convert Aggregator feeds into entities will open the opportunity to replace aggregator categorization with taxonomy in a very simple way: Aggregator items could be listed by their feed's taxonomies. The only setting necessary would be to assign which vocabulary to use for generating listings of aggregator items.
This approach will support categorization on a feed level, but not on a feed item level. I personally think this limitation would be fine.
Objections?
Comment #4
catchThe main question I have is what to do about the existing aggregator URLs - would we create new vocabularies for the existing categories and use hook_term_path() maybe?
Comment #5
Jody LynnComment #6
ParisLiakos CreditAttribution: ParisLiakos commentedI think #293318: Convert Aggregator feeds into entities will make this easier...attach a taxonomy reference field on aggregator feeds?
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedDries (and I) have indicated support for this at #293318: Convert Aggregator feeds into entities. Hope someone takes this on.
Comment #8
ParisLiakos CreditAttribution: ParisLiakos commentedi believe this falls into tasks now
Comment #9
BerdirYes, agreed that this should be considerably easier to do now.
Not sure if this should be postponed on the conversion of taxonomy term reference fields to ER. Not sure, maybe ask Amitaibu/amateescu if they see any problems.
I think one question here is how we take over the categories from the feed to the feed item as they are created.
And if the category should be a hardcoded field or a dynamic one that is just defined by a profile or something. We could no longer provide default items per category blocks and pages but you'd have to create them with views. But that's where we want to go anyway (although maybe with a views wizard or something).
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedI think we should create this field in
standard profileaggregator.install if taxonomy is enabled. i hate hardcoded fieldsThis way we give people the possibility to remove them..not everyone uses feed categories.
The category pages would still be available in
taxonomy/term/*
right? (Edit: No,taxonomy/term/*
lists nodes only)I am not sure about the blocks, but maybe create a view block which lists latest X items. Then it would be as simple as adding a views display in the same view and then a category condition in the filters.
We already have the feed entity in item creation, so it wont be hard to assign the same category to items.
What i am unsure what to do with: If i look at feed/feed item functionality, i can see that in the Categorize tab of a feed, someone can set different category per feed item alone..
Comment #11
BerdirNow that an item is an entity, it should be quite easy to add a form controller.
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commentedComment #13
ParisLiakos CreditAttribution: ParisLiakos commentedshow stopper #1935974: Cant attach entity reference fields to EntityNG
Comment #14
jibranAs per #13
Comment #15
Gábor HojtsyIt would be amazing if this could be done. The current category system in aggregator does not support multilingual in any way as far as I know and it is neither a content nor config entity. So it is in the dark lands of non-standard implementation with no common tooling support (not just only multilingual). Tagging for multilingual but no promises whatsoever that this would help anyone pick this up.
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedhello Gabor, i already started this, i am at about the 40% of the patch..it is gonna be quite big..if anyone wants to give a hand, ping me on irc, and i will create a sandbox
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commented#1942106: Entity reference and revisions
Comment #18
catchThat was marked duplicate of #1847596: Remove Taxonomy term reference field in favor of Entity reference.
Comment #19
Gábor Hojtsy@rootatwc: even if your patch does not work, posting it here would be fabulous so others can take a look (or take it over if you happen to get too frustrated and run far away :).
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedwell, it is not that it is not working, it just is work in progress..anyway, here is my work so far..i just started fixing tests and polishing stuff
It also contains the entity reference fix from the other issue
Comment #21
podarokhttp://drupal.org/node/1929006
\Drupal::service
Comment #22
BerdirAnd it will change again to Drupal::entityManager(), so it has to be changed again anyway => Don't bother.
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedactually this patch is not quite ready for review, it is a waste of time;)
and i think since this #1956434: Can't edit nodes with entity reference fields got committed i can pick this back..i hope i wont find more blockers
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedanother one
#1957888: Exception when a field is empty on programmatic creation
i am not gonna postpone this though, i ll hack field_sql_storage a bit
Comment #25
ParisLiakos CreditAttribution: ParisLiakos commentedlets see tests
to make things easier and patch smaller for now, i made taxonomy and er requirements
Comment #27
BerdirJust a quick question.
Hm, we do we need bundles for this? Can't we convert it using a single bundle first and then about that functionality in a separate step?
Comment #28
ParisLiakos CreditAttribution: ParisLiakos commentedYou are probably right, but i have no clue where to expose the UI..and i have to expose it somewhere
because of this
May the current admin path, although it ll get crowded a bit
Comment #29
ParisLiakos CreditAttribution: ParisLiakos commentedno bundles, tests will fail again
Comment #30
twistor CreditAttribution: twistor commentedInteresting, the missing route for 'aggregator_feed_add' makes all the menu entries fail.
Comment #31
ParisLiakos CreditAttribution: ParisLiakos commenteduh, sorry, just remove this entry..i will soon post a new patch, with this fix and upgrade path tests included
Comment #32
ParisLiakos CreditAttribution: ParisLiakos commentedUpgrade tests added, i am pretty sure there are still some tests that fail
Comment #34
ParisLiakos CreditAttribution: ParisLiakos commentedi have no clue why module tests broke -.- and they are soooo slow to try locally:/
Comment #36
ParisLiakos CreditAttribution: ParisLiakos commentedhopefully this passes
Comment #37
dawehnerGreat work so far!
I'm wondering whether it would be possible to not run N EQs. Can't you run first the one for the terms, and then pass in all term IDs on the aggregator_item one?
Comment #38
twistor CreditAttribution: twistor commentedWOOT!
#37, I don't think it can be done, maybe with entity_query_aggregate()? I'm not sure. But at least it should have a
->count()
call.Comment #39
twistor CreditAttribution: twistor commentedActually, this should do it. The result will have to be re-keyed, but that's all.
Comment #40
twistor CreditAttribution: twistor commentedSeems like there should be a drupal-wide generic function for this.
Could the field be named 'aggregator_categories'?
entity_query()?
arg()!
Comment #41
twistor CreditAttribution: twistor commentedThe behavior of the save buttons on aggregator/sources/%/categorize is messed up.
The individual save buttons don't seem to be working. Seems like the ItemFormController() needs a save() method.
Comment #42
ParisLiakos CreditAttribution: ParisLiakos commentedwoot i just found out entity_query_aggregate:)
Thats what i started doing. but then prefixed it with field_ so i can distinguish it from the
aggregator_categories
vocabularywoot! kill me now!
This stupid form is killing me...the day i replace it with views i am gonna get drunk:)
Comment #43
ParisLiakos CreditAttribution: ParisLiakos commentedYou mean for a key value of id, category name? why?
Or for a category load? if yes, i think we try to get rid of entity_load* wrappers, see #1757586: Remove MODULE_load*() & Co functions in favor of entity_*() functions
Fix quite some stuff..good news is, if user decides to delete aggregator categories vocabulary, there are no categories menu link and ctegories table in admin
Comment #44
ParisLiakos CreditAttribution: ParisLiakos commentedforgot interdiff
Comment #46
twistor CreditAttribution: twistor commentedI meant mapping a list of entities to
$entity->id() => $entity->label()
Comment #47
twistor CreditAttribution: twistor commentedThe failure in #43 is due to getting rid of arg(). I'm not sure what the future is supposed to bring exactly. Maybe,
$router_item['map'][2]
.Comment #48
ParisLiakos CreditAttribution: ParisLiakos commentedi cannot reproduce locally:/
#43: drupal-aggregator_taxonomy-15266-43.patch queued for re-testing.
Comment #49
ParisLiakos CreditAttribution: ParisLiakos commentedi lied
Comment #50
andypostMostly the patch is ready. What's left?
I think all queries should be normalized there's a lot of places which loads vocabulary or term then queries another entity...
Also there's some mark-up nitpicks mostly related to
read_more
themingThis check looks too expensive, select 1 from limit 1 is enough
this one require follow-up to remove markup and convert to theme('more_link')
why? maybe better use #access
entity_load_multiple_by_properties against 2 queries
$id and ->id() is different?
Comment #51
tim.plunkettSo that we don't introduce any extremely confusing bugs later, I think this should be
end($router_item['page_arguments'])
instead. That way the value isn't removed.Comment #52
ParisLiakos CreditAttribution: ParisLiakos commentedthanks for the reviews!
timplunkett: you couldnt be more right:)
i think my hack below makes this uncommitable
Comment #53
ParisLiakos CreditAttribution: ParisLiakos commentedone more place to fix per andypost's feedback
Comment #54
twistor CreditAttribution: twistor commentedentity_load_multiple_by_properties() always returns an array, so the if should be unnecessary.
We should use entity_query_aggregate() here to avoid doing N queries. Plus, make it a count() query. See #39.
:( This could be done with a simpler query than you had, and you don't need to load the vocabulary. But, this is definitely the fastest.
This should be loaded ahead of time with entity_load_multiple().
Can we change this to menu_get_object()?
load_multiple()
Almost any looping on entity_load() or entity_delete() should be a multiple instead.
Comment #55
ParisLiakos CreditAttribution: ParisLiakos commentedSo loop over items, get all fids, load multiple and then loop again in items and assign it to them? feels a bit ugly to me:P
here is a patch that fixes the rest (besides aggregation query)
i think i have nothing more to add to this patch, thus unassigning
Comment #56
twistor CreditAttribution: twistor commentedIt's not the prettiest, but it's better to do more loops and less queries.
Trying out the entity_query_aggregate().
Comment #57
twistor CreditAttribution: twistor commentedAdded the entity_query_aggregate() call in aggregator_view() and also changed the duplicate feed checking to use entity_query().
Comment #58
dawehnerI really like that usage!. Let's better use Drupal::service('entity.query')->getAggregate('aggregator_item')
Comment #59
ParisLiakos CreditAttribution: ParisLiakos commentedthe real status:(
#1957888: Exception when a field is empty on programmatic creation
Comment #60
twistor CreditAttribution: twistor commented@dawehner should we switch the entity_query() calls to Drupal::entityQuery()?
Comment #61
ParisLiakos CreditAttribution: ParisLiakos commentedyeah we should
#1957148: Replace entity_query() with Drupal::entityQuery()
Comment #62
twistor CreditAttribution: twistor commentedQuick find and replace.
Comment #63
ParisLiakos CreditAttribution: ParisLiakos commentedfor bundles i just opened #1966070: Make aggregator feeds bundleable - move plugins configuration per bundle
Comment #64
Gábor HojtsyTagging for D8MI.
Comment #65
jibranAs per #59
Comment #67
ParisLiakos CreditAttribution: ParisLiakos commentedComment #68
ParisLiakos CreditAttribution: ParisLiakos commentedwas quite hard to catch up...so many changes!
also a found a couple more bugs, one in the taxonomy term entity reference plugin:
and a typo in the FileldOverview class
Comment #70
ParisLiakos CreditAttribution: ParisLiakos commented#1998992: Typo in FieldOverview.php was really fast:P fixed the typo
Comment #72
ParisLiakos CreditAttribution: ParisLiakos commentedSo..In module enable/disable test, forum_uninstall fails.
When calling field_purge_batch() an exception is thrown that an invalid instance was specified...and well since this will take me a while to debug, since i cant reproduce manually yet, and this happens to be the slowest webtest ever..i will try to pick it up in weekend
here is with the rest failures fixed
Comment #74
ParisLiakos CreditAttribution: ParisLiakos commentedsigh..#2004316: Exception on field_purge_batch() after deleting a field
Comment #75
tim.plunkettBlocks removal of MENU_LOCAL_ACTION
Comment #76
jibranThis should happen before July 1st it is an API change?
Comment #77
ParisLiakos CreditAttribution: ParisLiakos commentedyeah, this is probably not gonna happen until D9
Comment #78
kim.pepperAs this has been postponed, I've created a new issue for #2046303: Convert aggregator_form_category to FormInterface and updated the meta issue
Comment #79
ParisLiakos CreditAttribution: ParisLiakos commentedComment #80
ParisLiakos CreditAttribution: ParisLiakos commentedthe blocker is in, but i guess still 9.x material since its an API change
Comment #81
jibranCan we update the issue summary with api changes and let committer decide weather it is D8 or D9 metrial?
Comment #82
Gábor HojtsyI agree it would be great to let committers decide.
Comment #83
ParisLiakos CreditAttribution: ParisLiakos commentedagreed!
updated the issue summary
Comment #83.0
ParisLiakos CreditAttribution: ParisLiakos commentedUpdated issue summary.
Comment #84
ParisLiakos CreditAttribution: ParisLiakos commentedand the correct status
Comment #84.0
ParisLiakos CreditAttribution: ParisLiakos commentedUpdated issue summary
Comment #84.1
ParisLiakos CreditAttribution: ParisLiakos commentedUpdated issue summary
Comment #85
sunFYI: The intermediate step of first doing #2127725: Remove category handling from aggregator has quite some support and is RTBC, pending @Dries's approval.
(IMO) The removal does not imply that we cannot or should not continue to explore the proposed solution here for D8. Much rather, it allows us to think about the best possible (re-)implementation from a clean slate, leveraging the best tools and architecture available.
Comment #89
sarmiliboyz CreditAttribution: sarmiliboyz commentedComment #99
quietone CreditAttribution: quietone at PreviousNext commentedThe
aggregator
module has been removed from Core in10.0.x-dev
and now lives on as a contrib module. Issues in the Core queue about theaggregator
module, like this one, have been moved to the contrib module queue.Comment #100
larowlanComment #101
larowlanComment #102
Eric_A CreditAttribution: Eric_A at RIVM, Dutch Open Projects commentedComment #103
dcam CreditAttribution: dcam as a volunteer commentedI'm closing this as being outdated. In the 9.5 years since the categories were removed no one has requested that they be restored, complained about not being able to bring them over from an older D7 site (at least not in this issue), or bothered to try and reroll the old patch. Because categories were removed "rerolling" isn't even an accurate term for what would have to happen. Practically speaking, re-adding categories would be a feature request because it means asking the maintainers and community to create and support something that currently isn't a feature. So in the absence of significant feedback from users requesting the return of categories I'm not inclined to work on this.
If anyone does want them, then I'd prefer for a new issue to be created as a feature request, including an upgrade path from older versions of Drupal. This one is too long and probably contains too much irrelevant content for future work.