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.
Update documentation definition for functions
Comment | File | Size | Author |
---|---|---|---|
#21 | interdiff-config.txt | 1.02 KB | vladan.me |
#21 | DrupalConfig-2.patch | 3.39 KB | vladan.me |
#18 | DrupalConfig-1.patch | 3.04 KB | vladan.me |
#18 | interdiff-1793990-#15-#17.txt | 1.97 KB | vladan.me |
#15 | DrupalConfig.patch | 1.5 KB | vladan.me |
Comments
Comment #1
-enzo- CreditAttribution: -enzo- commentedComment #2
chx CreditAttribution: chx commentedThanks for this patch, I miss the @return values in config greatly.
Please remove the @return on castValue, it's an array or a string not a string but hopefully #1653026: [META] Use properly typed values in module configuration will fix that for once and all.
Past that the only problem I see here is the wording on get() -- I will think on it.
Comment #3
-enzo- CreditAttribution: -enzo- commentedLeave castValue alone for now
Comment #4
chx CreditAttribution: chx commentedComment #5
chx CreditAttribution: chx commentedComment #6
jhodgdonThis can't be right:
Either the first line or the added @return is incorrect. I didn't look through the rest of the patch, but it looks like it was just generated by pasting the same @return for each function -- it needs to be done a bit more carefully. Thanks!
Comment #7
socketwench CreditAttribution: socketwench commentedFixed documentation on getName().
Comment #8
chx CreditAttribution: chx commentedThis looks good to me now and thanks for the work enzo and socketwench!
Comment #9
webchickJennifer's got this one. :)
Comment #10
jhodgdonI've committed this patch, as everything in it looks good.
But the file is not still up to our coding standards... As a follow-up, would someone like to go through and fix these items:
a) Berb tense in the function first lines? For instance:
Should be Dispatches. There are several others that should be fixed.
b) In the get() method, the explanation should go before the @param, not between the @param and the @return, and the @see should go at the end.
c) There are also a couple of minor wording/grammar things that could be fixed, such as:
Should be "Sets a value...".
d) The castValue() method needs a @return but doesn't have one (it has two @params, and I think the second should be @return).
e) After this patch, not all the param/returns have data types. still.
Comment #11
jhodgdonOh, not my issue now. :)
Comment #12
chx CreditAttribution: chx commentedDo not try to fix d). castValue is scheduled to be removed, it's an abomination, so please disregard that one.
Comment #13
socketwench CreditAttribution: socketwench commentedI'll try to update the patch after work today.
Comment #14
alexpottComment #15
vladan.me CreditAttribution: vladan.me commentedI have followed instructions from #10, here's report:
a) Fixed.
b) I can't really find @see, seems that was changed in meanwhile, also, about @param and explanation, that is explanation for that specific parameter, not for whole function, so maybe it should remain there?
c) Fixed.
d) Not applicable.
e) I didn't really find any @param or @return that doesn't have data type.
Comment #16
jhodgdonThese changes all look pretty good! I noticed you changed "config" to "configuration", which is excellent, but missed one:
There is also one in $isLoaded, and another in the @return of isNew(), and one in the get() method... etc.
And I agree, many of the comments in #10 were fixed between when I made them and now, so they don't apply.
There are also still a couple of functions that do not have param docs: validateName(), notify()
Thanks!
Comment #17
vladan.me CreditAttribution: vladan.me commentedThank you for reviewing patch, I've made those changes, can you please have another look? I'm attaching patch&interdiff.
Comment #18
vladan.me CreditAttribution: vladan.me commentedComment #19
jhodgdonThanks, we're just about there!
- Can you put "a" in the clear description: "Unsets *a* value"?
- validateName() does not have @param $name documentation.
- I don't think the set() method $value parameter is actually of type 'string'... can't it be other types of data?
Comment #20
vladan.me CreditAttribution: vladan.me commentedI really don't know, who we should ask?
Comment #21
vladan.me CreditAttribution: vladan.me commentedUploading patch according to #19
I've asked on irc, and got response that "mixed" should be valid information instead of string.
Comment #22
jhodgdonThank you! I think this is ready to get committed now.
Comment #23
Xano21: DrupalConfig-2.patch queued for re-testing.
Comment #24
jhodgdonThanks again! Committed to 8.x (minus one stray extra line).