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.
Class \Drupal could use some cleanup, as a preparation for
#2363341: Throw exception in Drupal::service() and friends, if container not initialized yet.
Goals:
- Add missing docblocks, fix wrong ones.
- Make the IDE 100% happy, to make it work for us, not against us.
- Add relevant @see to increase the DX when looking at it with an IDE.
Beta phase evaluation
Unfrozen changes | Unfrozen because it only changes documentation |
---|
Comment | File | Size | Author |
---|---|---|---|
#12 | D8-Drupal-cleanup-2363523-12.patch | 2.17 KB | donquixote |
Comments
Comment #1
donquixote CreditAttribution: donquixote commented\Drupal::url() docblock needs work. Otherwise I think this is solid. IDE is happy.
Changes:
\Drupal::$container
can beNULL
. Let's be honest about that in the docblock.(This will change in #2363341: Throw exception in Drupal::service() and friends, if container not initialized yet., but we are not there yet.)
\Drupal::getContainer()
may returnNULL
. Let's be honest about that in the docblock.\Drupal::url()
needs parameter docblocks.@see
alone is not enough, because the IDE can do nothing with it.Copied from
UrlGeneratorInterface::generateFromRoute().
It also needs a description update - I am open for suggestions.
Also, some of the
@see
statements should probably be kicked out in favor of@see \Drupal\Core\Routing\UrlGeneratorInterface::generateFromRoute()
\Drupal::l()
needs parameter docblocks.@see
alone is not enough, because the IDE can do nothing with it.Copied from
LinkGeneratorInterface::generate()
, but not in full detail.Comment #2
jhodgdonLooks mostly OK, thanks!
A few things to fix:
a)
Second line needs to end in .
b)
Second line needs to end in .
c)
Type says string; docs say it can be a string or array?
d)
Second line needs to end in .
e)
Why remove this @see?
Comment #3
er.pushpinderrana CreditAttribution: er.pushpinderrana commentedUpdated patch as per #2, please review once.
Comment #4
dawehnerAm I drunken or am I? These methods take url objects not route name and parameters and options
Comment #5
donquixote CreditAttribution: donquixote commented@jhodgdon (#2):
I took the description from LinkGeneratorInterface, but the type was already there. Apparently the two mismatch.
LinkGenerator::generate() allows a render array. But then calls drupal_render() in global scope. Beh.
Maybe we should simply say that \Drupal::l() only works with strings, no matter what LinkGeneratorInterface::generate() does.
I don't see what the
@see \Drupal\Core\Url
should be good for.The "Url" is already in the type specifier of the parameter. You can click on that with your IDE and get to the Url class, just as you would with the @see.
On the other hand, @see \Drupal\Core\Utility\LinkGeneratorInterface::generate() is a much more useful info, because the $container->get('link_generator') does not reveal what kind of object it returns.
Maybe more interesting than a
@see \Drupal\Core\Url
would be a quick hint how you can create Url objects.Not saying this is the ultimate wisdom, but this was my motivation anyways.
@dawehner (#4):
I don't know what was originally intended for these methods. But we can simply look into
\Drupal\Core\Routing\UrlGeneratorInterface::generateFromRoute()
and\Drupal\Core\Utility\LinkGeneratorInterface::generate()
, and their default implementations, to find how they are currently implemented.\Drupal::url() takes 3 strings and returns a string.
\Drupal::l() takes a string and a Url object, and returns a string.
Considering that, i find the docblock description of
\Drupal::url()
, and the mention of\Drupal\Core\Url::*
methods, confusing.UrlGenerator::generateFromRoute)
does not really do anything with\Drupal\Core\Url
.I don't really know what to do hear, I cannot read the mind of the author of these methods.
But the description needs to change.
Comment #6
donquixote CreditAttribution: donquixote commented@dawehner: Do you still have concerns with this?
Otherwise #3 is RTBC for my taste.
Comment #7
Fabianx CreditAttribution: Fabianx commentedI am pretty sure this needs a re-roll now that l() is deprecated to _l().
Comment #8
donquixote CreditAttribution: donquixote commentedThe patch in #3 still applied. But I just swapped two lines of @see to make the diff smaller.
This reintroduces the @param and @return that were removed in #2347831-7: Fix documentation for \Drupal::url(), \Drupal::l(), etc. (and fix the change record).
Comment #10
dawehner+1
This itself is a small improvement.
Comment #11
alexpottThis is optional - no?
This issue is a normal bug so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.
Comment #12
donquixote CreditAttribution: donquixote commentedI somehow had the idea that we no longer do the "(optional)". Another look at https://www.drupal.org/coding-standards/docs shows I am wrong.
And a look at UrlGeneratorInterface::generateFromRoute() shows the "(optional)" for $parameters is missing there too.
There are more places in class Drupal where the "(optional)" is missing, but I will only fix the one place that the patch in #8 touches.
Added to issue summary.
EDIT: Oops. (see #14 below)
Comment #13
dcrocks CreditAttribution: dcrocks commentedChanged status
Comment #14
alexpottFixing the beta evaluation.
Comment #15
jhodgdonLooks good and thanks for the beta eval.
Comment #18
jhodgdonComment #21
jhodgdonComment #23
jhodgdonWeird. This test is failing on:
Output: [PHP Parse error: syntax error, unexpected '[', expecting ')' in /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/react/promise/src/functions.php on line 62
?!?
I will just set this docs-only patch to RTBC and hopefully it will just be OK.
Comment #25
donquixote CreditAttribution: donquixote commentedThis is indeed weird.
It should only happen in PHP 5.3, where the short array syntax was not available. But the test log says we are in PHP 5.4.
Maybe the active PHP version is not the same as intended in the config array?
I created this testbot issue:
#2400237: Testbot should report exact PHP version.
Comment #26
donquixote CreditAttribution: donquixote commentedjthorson told me this specific test bot was on PHP 5.3 instead of 5.4. He somehow managed to assign a different bot to this issue, and now it works.
I take the freedom to re-rtbc this.
It was my patch, but it has already been sufficiently rtbc'd by others.
Comment #27
alexpottCommitted 71502cc and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the summary.