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

Reference: https://www.drupal.org/core/beta-changes
Unfrozen changes Unfrozen because it only changes documentation
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

donquixote’s picture

Status: Active » Needs review
FileSize
2.31 KB

\Drupal::url() docblock needs work. Otherwise I think this is solid. IDE is happy.

Changes:

  • \Drupal::$container can be NULL. 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 return NULL. 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.
jhodgdon’s picture

Status: Needs review » Needs work

Looks mostly OK, thanks!

A few things to fix:

a)

+   * @param string $route_name
+   *   The name of the route

Second line needs to end in .

b)

+   * @param array $options
+   *   (optional) An associative array of additional options,

Second line needs to end in .

c)

+   * @param string $text
+   *   The link text for the anchor tag as a translated string or render array.

Type says string; docs say it can be a string or array?

d)

+   * @param \Drupal\Core\Url $url
+   *   The URL object used for the link,

Second line needs to end in .

e)

-   * @see \Drupal\Core\Url
    */
   public static function l($text, Url $url) {

Why remove this @see?

er.pushpinderrana’s picture

Status: Needs work » Needs review
FileSize
2.34 KB
1.27 KB

Updated patch as per #2, please review once.

dawehner’s picture

+++ b/core/lib/Drupal.php
@@ -469,6 +469,17 @@ public static function urlGenerator() {
+   * @param string $route_name
+   *   The name of the route.
+   * @param array $route_parameters
+   *   An associative array of parameter names and values.
+   * @param array $options
+   *   (optional) An associative array of additional options.
+   *
+   * @return string
+   *   The generated URL for the given route.
+   *
+   * @see \Drupal\Core\Routing\UrlGeneratorInterface::generateFromRoute()
    * @see \Drupal\Core\Url
    * @see \Drupal\Core\Url::fromRoute()

@@ -493,8 +504,16 @@ public static function linkGenerator() {
-   * @see \Drupal\Core\Utility\LinkGeneratorInterface::generate()
+   * @param string $text
+   *   The link text for the anchor tag.
+   * @param \Drupal\Core\Url $url
+   *   The URL object used for the link.
+   *
+   * @return string
+   *   An HTML string containing a link to the given route and parameters.
+   *
    * @see \Drupal\Core\Url
+   * @see \Drupal\Core\Utility\LinkGeneratorInterface::generate()
    */
   public static function l($text, Url $url) {

Am I drunken or am I? These methods take url objects not route name and parameters and options

donquixote’s picture

@jhodgdon (#2):

Type says string; docs say it can be a string or array?

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.

    $variables = array(
      // @todo Inject the service when drupal_render() is converted to one.
      'text' => is_array($text) ? drupal_render($text) : $text,

Maybe we should simply say that \Drupal::l() only works with strings, no matter what LinkGeneratorInterface::generate() does.

Why remove this @see?

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):

Am I drunken or am I? These methods take url objects not route name and parameters and options

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.

donquixote’s picture

@dawehner: Do you still have concerns with this?
Otherwise #3 is RTBC for my taste.

Fabianx’s picture

Issue tags: +Needs reroll

I am pretty sure this needs a re-roll now that l() is deprecated to _l().

donquixote’s picture

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

I don't understand why @param and @return docs were removed from \Drupal::url() and \Drupal::l().
These docs are meant at least 50% for the machine (the IDE, mostly), which can't do anything with the "See \Drupal\Core\Url::fromRoute() for detailed documentation."

The proposed changes in #2363523: Docblock / cleanup in \Drupal will re-introduce the @param and @return, albeit with only a short description, and the "for detailed documentation, see ..." still in place.

The other question is, for url(), why do we refer to \Drupal\Core\Url::fromRoute(), and not to \Drupal\Core\Routing\UrlGeneratorInterface::generateFromRoute(), which is the actual function being called?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

+1

This itself is a small improvement.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update
+++ b/core/lib/Drupal.php
@@ -478,6 +478,17 @@ public static function urlGenerator() {
+   * @param array $route_parameters
+   *   An associative array of parameter names and values.

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

donquixote’s picture

This is optional - no?

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

This issue is a normal bug so we need to outline how it fits within the allowable Drupal 8 beta criteria.

Added to issue summary.
EDIT: Oops. (see #14 below)

dcrocks’s picture

Status: Needs work » Needs review

Changed status

alexpott’s picture

Issue summary: View changes

Fixing the beta evaluation.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good and thanks for the beta eval.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: D8-Drupal-cleanup-2363523-12.patch, failed testing.

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: D8-Drupal-cleanup-2363523-12.patch, failed testing.

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: D8-Drupal-cleanup-2363523-12.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: D8-Drupal-cleanup-2363523-12.patch, failed testing.

donquixote’s picture

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

donquixote’s picture

Status: Needs work » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs issue summary update

Committed 71502cc and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the summary.

  • alexpott committed 71502cc on 8.0.x
    Issue #2363523 by donquixote, er.pushpinderrana: Docblock / cleanup in \...

Status: Fixed » Closed (fixed)

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