API page: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%...
The documentation for Select::extend says to use the base name of the extending class, e.g. PagerSelectExtender. However this doesn't work, and in any case, core uses the full namespace as well, e.g. Drupal\Core\Database\Query\PagerSelectExtender.
This could also be a user-bug, in that I'm presuming the base name to be the last part of the namespace, analogous to basename(). I haven't found any documentation saying 'this part is the base name, the whole thing is called the fully qualified namespace', etc.
Comment | File | Size | Author |
---|---|---|---|
#12 | interdiff-2722763-09-12.txt | 2 KB | drnikki |
#12 | 2722763-extender-clarification-12.patch | 2.05 KB | drnikki |
#9 | interdiff-2722763.txt | 963 bytes | drnikki |
#9 | 2722763-extender-clarification-08.patch | 1.09 KB | drnikki |
#6 | 2722763-extender-clarification.patch | 1.09 KB | drnikki |
Comments
Comment #2
jhodgdonAgreed, this is confusing, and you do indeed need to pass in the full namespace of the extender you want to use, without the leading \ (so it is not the "fully-qualified namespace, which would start in \ ).
Should be a good novice project to fix up the docs...
Comment #3
claw CreditAttribution: claw commentedAs far as terminology goes, the PHP documentation describes both \my\name and my\name as 'fully qualified' (e.g. PHP namespaces and dynamic language features).
The only other (quickly grepped) references in the codebase to 'base name' are 'database name', or 'base namespace', the latter of which is used in AnnotatedClassDiscovery.php to map namespaces to directories.
So I think it is just that one example, and it should be changed to something like...
$extender_name: The fully-qualified name of the extending class, without the leading '\'. The extender name will be checked against the current database connection to allow driver-specific subclasses as well, using the same logic as the query objects themselves.
Thoughts?
Comment #4
drnikki CreditAttribution: drnikki as a volunteer commented@claw I think that makes sense, and in theory, if someone's using this they don't need an example, but my first instinct would be to add an example, like:
$extender_name: The fully-qualified name of the extending class, without the leading '\', for example Drupal\my_module\myExtendingClass. The extender name will be checked against the current database connection to allow driver-specific subclasses as well, using the same logic as the query objects themselves.
Happy to patch this either way, though.
Comment #5
claw CreditAttribution: claw commentedI agree with @drnikki; if there had been an example, I probably wouldn't have raised this issue at all!
Comment #6
drnikki CreditAttribution: drnikki as a volunteer commented@claw Patch rolled and applies to 8.2.x but I'm guessing would also apply to 8.1.x but haven't tested it yet.
Comment #7
jhodgdonWorks for me! This seems like very good documentation. Two small things to fix:
extending -> extender
Extra space between . and The
Comment #8
claw CreditAttribution: claw commentedThe second sentence should also say 'The extender name will be...' rather than reintroducing the misleading 'The base name will be...'
But yes, thanks for doing this.
Comment #9
drnikki CreditAttribution: drnikki as a volunteer commented@claw, good call!!
@jhodgdon thanks!! Updated the extender text and the spacing. Damn my old habit of two spaces after every period! :)
interdiff attached, also. I generated it backwards but, it's there.
Comment #10
drnikki CreditAttribution: drnikki as a volunteer commentedComment #11
jhodgdonThanks! Almost perfect:
This line went over 80 characters; needs to be rewrapped.
And I know what you are saying about two spaces after the . -- you must have taken the same 8th grade typing class I had. ;)
As long as we are fixing this doc block, maybe we could also add a blank line between the @param and @return?
Comment #12
drnikki CreditAttribution: drnikki as a volunteer commented@jhodgdon, I sure did. And I've been a die-hard two-spacer ever since.
SOOOO, since I was fixing up the doc block, I sure did add the blank line before the @return, but I also went ahead and wrapped the Interface description at 80 lines and removed the two spaces there. I can totally take that out, but I figured ... we're here. :) It's a small file.
Comment #13
claw CreditAttribution: claw commentedIs the other change in the patch (the paragraph after 'Interface for extendable query objects.') from the line wrapping to 80 columns?
Comment #14
claw CreditAttribution: claw commentedErr... I reread what you wrote. It is. I personally don't like it, as it's unrelated to this issue, but won't object to it being included.
Comment #15
drnikki CreditAttribution: drnikki as a volunteer commented@claw - I hear you, but since it's such a short file - with just those two function comments in it, and we were already adding the blank line, i figured it was harmless. It is unrelated, but I didn't see the harm in adding it nor the benefit in opening another issue just to fix it.
Comment #16
jhodgdonYes, a bit out of scope to fix the wrapping, but it's a small patch, so let's go ahead and do it. Thanks!
Comment #18
jhodgdonUnrelated Migrate test failure.
Comment #21
jhodgdonLooks like this was committed.
Comment #22
claw CreditAttribution: claw commentedIndeed it has. Thank you both.