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 #1310084: [meta] API documentation cleanup sprint
I am opening this issue and will submit a patch shortly!
Comment | File | Size | Author |
---|---|---|---|
#29 | drupal8.documentation.1316902-29.patch | 21.49 KB | Albert Volkman |
#29 | interdiff.txt | 1010 bytes | Albert Volkman |
#26 | select-qry-docs-1316902-26.patch | 21.48 KB | Albert Volkman |
#26 | interdiff.txt | 21.77 KB | Albert Volkman |
#24 | select-qry-docs-1316902-24.patch | 26.01 KB | Albert Volkman |
Comments
Comment #1
nicl CreditAttribution: nicl commentedI haven't had time for this in the end so if anyone else feels like having a look feel free.
Comment #2
xjmCrossposting #1255524: select.inc needs more doxygen.
Comment #3
xjmSee also #1336726: Document which database methods can be chained (make sure it's correct).
Comment #4
xjmApparently you can ninja-assign an issue with a crosspost. ;)
Comment #5
chris.leversuch CreditAttribution: chris.leversuch commentedI'll try this one.
Comment #6
jhodgdonThis issue has a start of a patch for select.inc (and I've marked it as a duplicate):
#1255524: select.inc needs more doxygen
Comment #7
Crell CreditAttribution: Crell commentedEep!
Um, not to dissuade anyone, but please see: #1321540: Convert DBTNG to namespaces; separate Drupal bits.
I'm moving around ALL of the code in the /database directory. That's absolutely going to collide with anything here, and make one or both of us manually reroll a ton of code. Let's please not do that. :-(
Can we mark this postponed on that issue?
Comment #8
xjmYeah, having read and tickled the 600k monster that is Crell's patch in #1321540: Convert DBTNG to namespaces; separate Drupal bits, I'd definitely suggest postponing this particular cleanup until after that issue goes in.
Comment #9
chris.leversuch CreditAttribution: chris.leversuch commentedLuckily I haven't had much time for this patch so I haven't done too much. I'll stop until the other patch is in.
Comment #10
jhodgdontagging
Comment #11
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedChanging this to active since #1321540: Convert DBTNG to namespaces; separate Drupal bits is closed.
Comment #12
jhodgdonI'm going through these issues and un-assigning any without activity for several weeks. If you still want to work on this issue, please feel free to assign it back to yourself. Thanks!
Comment #13
TravisCarden CreditAttribution: TravisCarden commentedThe core/includes/database directory no longer exists in d8. Does anything need to be done in this issue before closing it and removing it from the meta issue?
Comment #14
Crell CreditAttribution: Crell commentedRetitling. :-)
Comment #15
larowlanI'd already started on this over in #1760820: no docs at all for many methods of SelectQuery, eg ::isNull/isNotNull
Comment #16
larowlanThis is cleanup of select class.
Since all of the database classes have since been split into separate files in line with PSR-0, I propose we add follow-up tasks for the other classes.
Comment #17
jhodgdonRegarding follow-up tasks, that is not how we have been doing this API cleanup effort -- and we really don't want to have a separate issue for every file in Drupal Core to clean it up (pretty much every file in Drupal Core needed some cleanup!). That would be very tedious. So what we've been doing is one issue per module, one per theme, and then we split up the other parts of core to similar-sized groups of files.
So: we really do want one patch that cleans up all the database stuff, eventually.
I took a quick look at this patch, and it looks like you're not yet familiar with our documentation standards:
http://drupal.org/node/1354#functions
Check out the formatting we require for function/method docs and try again. :)
You should also read the meta-issue this is a part of:
#1310084: [meta] API documentation cleanup sprint
to find out what specifically we are cleaning up, before tackling any other classes in this issue.
Thanks!
Comment #18
jhodgdonForgot status change, and changing title back to what this issue is supposed to encompass
Comment #19
larowlanMost of the methods in the patch have been documented in their respective defining interface. Should I be duplicating these docblocks? Seems like overkill.
Comment #20
jhodgdonNo. You did fine on the "Implements name/of/interface" doc blocks, aside from a lot of them missing . at the end. Those are mostly like:
http://drupal.org/node/1354#hookimpl
I thought we had a standard written down somewhere for the class overrides/implements too, not sure where that is... but blocks like this are fine:
Comment #21
larowlanCleans up Query/Select.php and Query/SelectExtender.php
What's next?
Comment #22
jhodgdonWhat's next? This issue covers all of the directory core/lib/Drupal/Core/Database. Check that directory and see what else needs to be done.
But first, let's get this much right. A few things to fix:
a)
See http://drupal.org/node/1354#functions -- read those guidelines and follow them.
b)
Fix indentation -- docs comment should be indented same as "public" (2 spaces). I think the rest of the methods in this file are fine. Just do this one the same.
c)
Needs a one-line summary... but this isn't accurate anyway. You can only use the "Implements..." wording if something is an implementation of an interface method, and it looks like this isn't one. This applies to quite a few more methods in the patch too.
d) There are several other methods in the patch, that, like (a), need to be fixed up to follow our function guidelines, such as:
Comment #23
Albert Volkman CreditAttribution: Albert Volkman commentedRe-roll of #19 against current head. Still needs fixes from comments in #22.
Comment #24
Albert Volkman CreditAttribution: Albert Volkman commentedThis patch covers a & b from #22.
Comment #25
Crell CreditAttribution: Crell commentedIt looks like we'll be dropping the "Implements Foo\Bar\Baz" style comments shortly: #1906474: [policy adopted] Stop documenting methods with "Overrides …"
I know, the timing is terrible. :-(
Comment #26
Albert Volkman CreditAttribution: Albert Volkman commentedC'est la vie. Here we go for the majority of the comments. However, I'm not sure about what to do about the 'for the HAVING clause' lines. I don't think those would be able to figure out their proper parents (havingConditions vs conditions methods). Also, for-
Implements Drupal\Core\Database\Query\SelectInterface::join().
for method 'innerJoin'. What should be done there?
Comment #27
larowlanComment #28
jhodgdonThe param lines in the Select::__construct method do not have . at the end of their descriptions.
etc.
Also $options is *connection* options, not query options.
So... Regarding all the inheritdoc lines... You can only use inheritdoc if the class actually is implementing or overriding methods on an interface or class that it inherits from.
In this case, Select inherits (directly or indirectly) from:
Query
SelectInterface
ConditionInterface
AlterableInterface
ExtendableInterface
PlaceholderInterface
and maybe some more that those inherit from. So all of the @inheritdoc things are probably fine, and the lines like
should just be using inheritdoc as well, since those are methods on one of the interfaces. Right?
Comment #29
Albert Volkman CreditAttribution: Albert Volkman commentedI don't believe these docs are correct. Those methods do not exist in the interface-
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%...
Comment #30
jhodgdonCheck the interfaces more carefully -- this class inherits a LOT of interfaces (some via the base class). I think inheritdoc is right.
Comment #31
Albert Volkman CreditAttribution: Albert Volkman commentedAh, I see that now. Thanks for pointing it out @jhodgdon! I concur that inheritdoc is correct.
Comment #32
jhodgdonThese issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.
Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!