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.
Part of meta-issue #2016679: Expand Entity Type interfaces to provide methods, protect the properties.
See the detailed explanations there and look at the issues that already have patches or were committed.
Add get*, set* and additional methods as it makes sense to replace the public properties (e.g. isSomething() and something())
Comment | File | Size | Author |
---|---|---|---|
#29 | core-aggregator_item_properties-2028039-29.patch | 14.92 KB | amateescu |
#25 | core-aggregator_item_properties-2028039-25.patch | 14.98 KB | ParisLiakos |
#25 | interdiff.txt | 843 bytes | ParisLiakos |
#23 | drupal8.aggregator-module.2028039-23.patch | 14.5 KB | xeniak |
#19 | core-aggregator_item_properties-2028039-19.patch | 15.01 KB | mcrittenden |
Comments
Comment #1
alarcombe CreditAttribution: alarcombe commentedComment #2
YesCT CreditAttribution: YesCT commented@alarcombe if you had any work here, it's ok to post it, even it it's only partial. Or ask any questions you have.
Otherwise I think this is available. :)
Comment #3
alarcombe CreditAttribution: alarcombe commented@YesCT - sure, was eyeing this up wrt https://drupal.org/node/2016701 but won't have time over the next couple of weeks to look at it.
Comment #4
undertext CreditAttribution: undertext commentedComment #5
BerdirThanks for working on this, looks good, just a few minor things and one method rename.
method calls in code don't work, you could add {} around it, but I prefer '...' . $item->getFid().
Additional space in here.
Let's do getFeedId() and setFeedId() instead, which is much more self-explaining.
there needs to be an empty line between @param and @return.
Another double space.
Comment #6
jsbalseraComment #7
jsbalseraIt needed a re-roll, also made the changes.
Comment #8
BerdirThose calls should now use FeedId() instead of Fid()
two empty lines here.
same here.
Comment #9
jsbalseraComment #10
jsbalseraComment #11
BerdirThanks, looks good to me!
Comment #12
ParisLiakos CreditAttribution: ParisLiakos commentedAgreed it looks good, +1 for (set|get)FeedId()
getFid() should be getFeedId()
null should be NULL (caps)
and !== instead of !=
But lets store it to a temporary variable to avoid same function call
otherwise looks good to me too, so leaving as RTBC.
Thanks!
Comment #13
johnennew CreditAttribution: johnennew commentedThis follows best practice on the conversion to methods.
+1 for RTBC
Comment #14
alexpottPatch no longer applies.
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedFieldInterface => FieldItemListInterface
Comment #16
Berdir#15: core-aggregator_item_properties-2028039-15.patch queued for re-testing.
Comment #18
johnennew CreditAttribution: johnennew commentedComment #19
mcrittenden CreditAttribution: mcrittenden commentedComment #20
ParisLiakos CreditAttribution: ParisLiakos commentedthanks for reroll
Comment #21
tstoecklerLooks good. For extra credit Item and ItemInterface could use an empty newline for the closing bracket of the class, but not setting to needs work for that. Could also be fixed pre-commit.
Comment #22
alexpottPatch no longer applies.
Comment #23
xeniak CreditAttribution: xeniak commentedReroll.
Comment #24
tim.plunkettRemoving tags
Comment #25
ParisLiakos CreditAttribution: ParisLiakos commentedreroll missed a spot. also removing an irrelevant change
Comment #26
johnennew CreditAttribution: johnennew commentedThis minor change looks good to go.
Comment #27
Xano#25: core-aggregator_item_properties-2028039-25.patch queued for re-testing.
Comment #28
alexpottPatch no longer applies.
Comment #29
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #30
alexpottCommitted bb54afd and pushed to 8.x. Thanks!
Comment #31
BerdirWe don't enforce that the methods are used and we already have a general change notice that base fields now have methods that should be used: https://drupal.org/node/1806650. I don't think we need to reference every single issue that adds them, added the meta issue..
Comment #32
alexpottComment #33
alexpottxpost :)
Comment #35
xjm