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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claw created an issue. See original summary.

jhodgdon’s picture

Issue tags: +Novice

Agreed, 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...

claw’s picture

As 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?

drnikki’s picture

@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.

claw’s picture

I agree with @drnikki; if there had been an example, I probably wouldn't have raised this issue at all!

drnikki’s picture

Status: Active » Needs review
FileSize
1.09 KB

@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.

jhodgdon’s picture

Status: Needs review » Needs work

Works for me! This seems like very good documentation. Two small things to fix:

  1. +++ b/core/lib/Drupal/Core/Database/Query/ExtendableInterface.php
    @@ -18,9 +18,10 @@
    +   *   The fully-qualified name of the extending class, without the leading '\'
    

    extending -> extender

  2. +++ b/core/lib/Drupal/Core/Database/Query/ExtendableInterface.php
    @@ -18,9 +18,10 @@
    +   *   (for example, Drupal\my_module\myExtendingClass).  The base name will be
    

    Extra space between . and The

claw’s picture

The 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.

drnikki’s picture

@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.

drnikki’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Thanks! Almost perfect:

  1. +++ b/core/lib/Drupal/Core/Database/Query/ExtendableInterface.php
    @@ -18,9 +18,10 @@
    +   *   (for example, Drupal\my_module\myExtenderClass). The extender name will be
    

    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. ;)

  2. +++ b/core/lib/Drupal/Core/Database/Query/ExtendableInterface.php
    @@ -18,9 +18,10 @@
    +   *   subclasses as well, using the same logic as the query objects themselves.
        * @return \Drupal\Core\Database\Query\ExtendableInterface
    

    As long as we are fixing this doc block, maybe we could also add a blank line between the @param and @return?

drnikki’s picture

@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.

claw’s picture

Is the other change in the patch (the paragraph after 'Interface for extendable query objects.') from the line wrapping to 80 columns?

claw’s picture

Err... 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.

drnikki’s picture

@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.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Yes, a bit out of scope to fix the wrapping, but it's a small patch, so let's go ahead and do it. Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 2722763-extender-clarification-12.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated Migrate test failure.

  • alexpott committed 53e54ea on 8.2.x
    Issue #2722763 by drnikki: Select::extend doc incorrectly says to use...

  • alexpott committed 5b88f21 on 8.1.x
    Issue #2722763 by drnikki: Select::extend doc incorrectly says to use...
jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Looks like this was committed.

claw’s picture

Indeed it has. Thank you both.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.