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.
This makes it easier to follow the flow of the code where sort() is called on the entity class with uasort().
Comment | File | Size | Author |
---|---|---|---|
#15 | 2084653-uasort-see-docs-15.patch | 2.72 KB | longwave |
#8 | 2084653-8.drupal.uasort-see-ConfigEntityBase-sort.patch | 2.71 KB | joachim |
#1 | 2084653.drupal.uasort-see-ConfigEntityBase-sort.patch | 2.71 KB | joachim |
Comments
Comment #1
joachim CreditAttribution: joachim commentedComment #2
longwaveMakes sense to me. Documentation-only patch.
Comment #3
jhodgdonSounds like a good idea. But if you refer to namespaces in documentation, you need to make them fully-qualified (start with a \ ).
Also longwave, when you reviewed this did you check carefully to make sure that all of the references were correct? It wasn't obvious to me whether or not the references to the classes were correct, just looking at the patch. So I'd just like a bit of reassurance. :)
Comment #4
joachim CreditAttribution: joachim commented> Sounds like a good idea. But if you refer to namespaces in documentation, you need to make them fully-qualified (start with a \ ).
Core has 183 occurrences of that!
I'll wait for a review of the classes before rerolling for the initial slash.
Comment #5
jhodgdonYes, Core is often not following our standards, but we do have one that says if you use namespaces in docs, they *must* be fully qualified.
https://drupal.org/node/1354#classes
The previous patch was reviewed, so you might as well fix it now? :)
Comment #6
joachim CreditAttribution: joachim commented> Yes, Core is often not following our standards
:D
> Also longwave, when you reviewed this did you check carefully to make sure that all of the references were correct?
I mean that bit. Makes more sense to wait for any comments on that front, and reroll then?
Comment #7
longwaveYes, I checked them all :) Details as follows:
ConfigEntityListController is a generic list controller for ConfigEntityBase objects, so ConfigEntityBase::sort() is correct.
BlockListController lists Block entities, which have their own overridden sort function, so Block::sort() is correct.
LanguageListController lists Language entities, which do not have their own sort function but inherit from ConfigEntityBase, so ConfigEntityBase::sort() is correct.
RoleStorageController handles Role entities, which again do not have their own sort function but inherit from ConfigEntityBase, so ConfigEntityBase::sort() is correct.
Comment #8
joachim CreditAttribution: joachim commentedUpdated patch.
I've removed the full stops from the end of the @see lines, as AFAICT that's the correct format.
Comment #9
longwaveLooks good. @see needs no trailing stop, though core does not always conform to this.
Comment #10
jhodgdonShould we even use @see in inline documentation? It has a specific use in /** */ comments because those turn into links and a See Also section... ??? It's probably better to just say "See blah blah blah" in in-line docs?
Comment #11
longwaveWe use it in enough other places already:
16 inline @see references to classes:
Comment #12
jhodgdonBut "we're doing it a lot" doesn't mean "it's a good idea to do it more" or "it's our standard". For instance, we're using the word "id" all over documentation in place of "ID", but it's still incorrect every time we do it.
My question is whether using @see in inline docs is a good idea, since "See" would be just as clear and is plain English, and our documentation standards generally say we should use plain English for documentation, and @ tags in /** */ headers, right?
Comment #13
longwaveIsn't that out of scope for this issue? Shouldn't we just get this improvement in, and then address that (and all other uses of // @see) in a followup?
Comment #14
jhodgdonUm... Let's follow our current standards in this patch, which say to write inline docs in plain English, and if you want to change the standards so that we encourage using @tags in inline docs, you can do that in another issue? :)
Comment #15
longwaveComment #16
longwaveI guess the old title is inaccurate :)
Comment #17
jhodgdonThank you. This looks fine to me.
Comment #18
jhodgdonThanks again- committed to 8.x.