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.
From 8.x-3.x.
Comment | File | Size | Author |
---|---|---|---|
#77 | drupal-1821844-77.patch | 35.63 KB | olli |
#77 | interdiff.txt | 1.59 KB | olli |
#73 | 1821844-73.patch | 35.89 KB | damiankloip |
#65 | 1821844-65.patch | 35.71 KB | jibran |
#65 | interdiff.txt | 10.64 KB | jibran |
Comments
Comment #1
xjmComment #2
xjmHandlerAllTest
still enables aggregator; need to double-check that its handlers are actually tested.Comment #3
xjmThis should be better for reviewing the test coverage.
Comment #4
xjmThat's broken. This isn't, and locally shows results for aggregator.
Comment #5
xjmI'll spin the patch for debugging messages off into a separate issue.
Comment #6
BerdirFYI, #293318: Convert Aggregator feeds into entities converts feed items to EntityNG, doesn't use a property table yet, however. so this shouldn't conflict much...
Comment #7
xjmThanks @Berdir.
Before anyone goes crazy reviewing and complaining about the odd inline comments, this is a straight port from 8.x-3.x and isn't really ready to be reviewed yet.
Comment #8
xjmMaking that more explicit. :)
Comment #9
xjmComment #10
dawehnerAnother issue which is blocked on #1783196: Get the views plugin manager from the DIC when possible (because you simple can't add a new view)
The diff in #4 got in somewhere else.
Comment #11
xjmSending to the bot.
Comment #12
dawehnerRerolled, updated all the codestyles, fixed a lot of the code.
Comment #14
dawehnerFix these errors: Init method has been wrong.
Comment #15
xjmOops, didn't mean to leave this assigned to me.
Comment #16
Berdir#14: drupal-1821844-14.patch queued for re-testing.
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commentedi think something is missing, that could allow us to replace
admin/config/services/aggregator
in the futureComment #18
dawehnerThat's a good idea, did you reviewed the patch?
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedNope, i didnt review it, i was just playing with views:P
I took a closer look now
I guess the first line needs to be removed..we could use entity_load_multiple but dunno, if it is worth it.
I dont see placeholders variable used after this..i guess one more leftover
i dont get why link is uppercase
Comment #20
dawehnerI don't know about the feed_LINK part but this has been part of the aggregator integration since views2.
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedAll i can complain about is
should be ->label()
same
Comment #22
dawehnerThere we go.
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commenteddo we need tests here?
Comment #24
dawehner#22: drupal-1821844-22.patch queued for re-testing.
Comment #25
dawehnerWorking on writing tests and fixing some minor issues.
Comment #27
dawehnerStarted to write some tests and fix some of the handlers.
Would be great to get #1956220: Allow to use xpath on drupal unit tests/unit tests so we can test the feed output, on the DUTB.
Comment #28
ParisLiakos CreditAttribution: ParisLiakos commentedMaybe remove the comment too?
Lets make this lowercase here, it is ugly as hell and i am pretty sure it wont break anything
oops
Comment #29
dawehnerComment #30
dawehnerThanks for the review, here is an interdiff.
Comment #31
ParisLiakos CreditAttribution: ParisLiakos commentedi see you removed the extra fields, so i guess if they are not needed, lets load the item using the entity storage?items are entities:)
Fill it or remove it:) also wrong docblock
Comment #32
dawehnerThere we go.
Comment #33
dawehner/me sighs about myself.
Comment #34
dawehnerSo this time I converted the rss.php to load the entity.
Comment #36
ParisLiakos CreditAttribution: ParisLiakos commentedsorry, i just noticed them
missing comma
comma as well
Comment #37
dawehnerThanks for the review. Fixed the exception as well.
Oh wow, I would have not exception that this patch is so big.
Comment #38
ParisLiakos CreditAttribution: ParisLiakos commentedthis looks awesome now:) thanks
RTBC!
Comment #39
webchick#37: drupal-1821844-37.patch queued for re-testing.
Comment #40
olli CreditAttribution: olli commentedOne extra space before =.
Another extra space here and in a dozen similar places.
Extra argument.
I think ->fetchAssoc() does not work here.
Path is aggregator/categories/$cid.
SELECT * -> SELECT cid, title
This is not used in tests. Should it use webtest if it is impossible to test feed until #1956220: Allow to use xpath on drupal unit tests/unit tests?
Comment #41
dawehnerWow you are an eagle!
Comment #42
damiankloip CreditAttribution: damiankloip commentedCouldn't we use fetchAllKeyed(0, 1) instead?
Comment #43
dawehnerInstead of fetchAllKeyed() or instead of fetchCol?
Comment #44
twistor CreditAttribution: twistor commentedHeads up, #15266: Replace aggregator category system with taxonomy is green.
Comment #45
dawehnerIs the other one commitable in a reasonable timeframe?
Comment #46
ParisLiakos CreditAttribution: ParisLiakos commentedit is no problem, whichever gets committed first, since views integration files are not touched from the other patch..this patch here just introduce new files...i can easily
git rm
the categories one...just please dont give any extra attention to the categories integration..it will die soonish:D
so if olli's concerns are resolved this should go back to rtbc
Comment #47
olli CreditAttribution: olli commentedI'd like to set this back to rtbc, but I found these:
Warning: Invalid argument supplied for foreach() in Drupal\aggregator\Plugin\views\row\Rss->render() (line 98)
Notice: Trying to get property of non-object in Drupal\aggregator\Plugin\views\argument\CategoryCid->title_query() (line 33)
Comment #48
damiankloip CreditAttribution: damiankloip commentedGood catches, the first warning I'm not sure about, because I don't know when elements would be there? It's not declared as a property or anything? I've wrapped it for now, but it could be removed?
I have also changed the argument title_query method as fetchCol will return a flat array of values, not an object. So $row->title will never exist. We can just use that array and return a mapped version of that.
Comment #49
dawehnerI fixed all consers beside the tests, as yeah the category integration will be dropped soon.
Comment #50
olli CreditAttribution: olli commentedlabel
I think we can remove that. In the original patch (or in d7) that is
$item->elements
which is set right before the foreach so there is no way elements could be there.Comment #51
damiankloip CreditAttribution: damiankloip commentedYep, that's what I thought, and it seems there is no plan to actually ever need it. Let's rip it out!
Comment #52
dawehnerJust fixing the label as well.
Comment #53
olli CreditAttribution: olli commentedGreat!
Comment #54
alexpottShould be using
aggregator_filter_xss()
And I think we a test to ensure aggregator_filter_xss() is being used as expected on the aggregrator item author and description.
Comment #55
wamilton CreditAttribution: wamilton commentedHere's the requested updates, assertions are failing now. I'd like some oversight on this.
Comment #56
wamilton CreditAttribution: wamilton commentedComment #58
olli CreditAttribution: olli commented@wamilton, I think there has been some changes in the annotations:
These have changed to @PluginID("aggregator_category_cid")
This is using display_types = {"feed"}
In addition to those changes:
This should also use aggregator_filter_xss() as pointed out by @alexpott.
That aggregator_filter_xss() takes only one parameter.
Comment #59
jibranFixed #58 but #54 is still pending.
Comment #60
dawehnerThank you!
As mentioned in the other issue, you can drop it.
What about injecting the db connection as well?
check_plain should be replaced by String::checkPlain now
Even more injections.
Same here.
Comment #61
jibranThanks @dawehner and @olli for the review. Fixed everything in #60.
Comment #62
damiankloip CreditAttribution: damiankloip commentedLooking pretty good!
I guess aggregator_xss is the correct id? or is the item description intentionally not using the Aggregator xss implementation on purpose?
Before, when we have the 'click sortable' keys in the data, that would disable click sorting for both description fields. The aggregator specific Xss field handler can just implement its own click_sortable method and
return FALSE
. If using views' xss handler for item description is correct then'click sortable' => FALSE
will need to be added back to the data.If this is extending Drupal\views\Plugin\views\field\Xss then sanitize value will always be passed 'xss' the type. So why have the fallback? I guess the actual type is pretty much irrelevant here, as we always want it run though this.
Comment #63
jibranConverted
'id' => 'xss',
forguid
to'id' => 'standard',
as per @dawehner suggestion.Added back
'click sortable' => FALSE,
that change was unnecessary. Good catch @damiankloip.Simplified
sanitizeValue
.Comment #64
dawehnerShould be {@inheritdoc}. While you are here, __construct should be better above, as that's THE important function here.
Needs docs, sorry.
Needs docs.
This one also could get a injection?
so views_view_row_css takes care about the escaping here.
Comment #65
jibranThanks you @dawehner for the review and help. Fixed everything in #64.
Comment #66
dawehnerWe have at least some basic testing, so that's good to go! Thank you very much for bring this home.
Comment #67
damiankloip CreditAttribution: damiankloip commentedHowcome we have added this to this patch? Doesn't seem like views integration :)
Comment #68
dawehnerIn order to test the timestamp rendering we have to be able to force a certain value, but the ItemStorageController just forced you to use the request all the time, so this seems to be a totally fine adapation.
Comment #69
damiankloip CreditAttribution: damiankloip commentedOK, makes sense. We should consider moving (follow up) this for all created properties as there have been similar problems in tests recently.
Comment #70
xjmAgreed, let's get a followup issue for that. :)
Comment #71
xjmComment #72
alexpottComment #73
damiankloip CreditAttribution: damiankloip commentedRerolled.
Comment #75
ParisLiakos CreditAttribution: ParisLiakos commented#73: 1821844-73.patch queued for re-testing.
Comment #77
olli CreditAttribution: olli commented#1893772: Move entity-type specific storage logic into entity classes
Comment #78
dawehnerThank you for the rerole!
Comment #79
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.