Unexpected API change in UrlGenerator::generate()

Let's ensure we have test coverage for the different available types ...

Tagging for release notes in case it's not fixed in time.

I'm assuming this breaks install in subdirectories, so bumping to major for that reason. If I've got that wrong and it's just a correct absolute vs. relative url then we could bump back to normal though.

In general its great that we got this committed so people find the issue.

Looking at the root issue, there is a missmatch between the Synfony UrlGeneratorInterface->generate() which expects $refernceType to be a constant and our implemenentation at UrlGenerator->generate() that even renames $refernceType to absolute and uses a boolean.

A have added two tests for absolute and relative URL generation (some part of them might be redundant as I am not familiar with the bubbling thing) and moved the fix from #2 to the generate method.

This is the same patch, but fixing the interface missmatch.

EDIT with more info.

public function generate($name, $parameters = array(), $reference_type = self::ABSOLUTE_PATH)

instead of

public function generate($name, $parameters = array(), $absolute = FALSE)

I have only run the tests with ./vendor/bin/phpunit --bootstrap ./core/tests/bootstrap.php core/tests/Drupal/Tests/Core/Routing/UrlGeneratorTest.php, and they pass with both patches.

Not sure why the new tests are failing. The messages in QA are these:

testAliasGenerationUsingInterfaceConstants
fail: [Other] Line 222 of core/tests/Drupal/Tests/Core/Render/MetadataBubblingUrlGeneratorTest.php:
--- Expected

testAbsoluteURLGenerationUsingInterfaceConstants
fail: [Other] Line 402 of core/tests/Drupal/Tests/Core/Render/MetadataBubblingUrlGeneratorTest.php:
--- Expected

Any idea? Test pass in my VM

Debugged the test locally. MetadataBubblingUrlGenerator needed exactly the same change as UrlGenerator. The test methods you added passed for that test, but this is a different test that inherits from the one you updated which is why it confusingly looks like the test is passing when you run just that one.

+++ b/core/lib/Drupal/Core/Render/MetadataBubblingUrlGenerator.php
@@ -96,8 +96,8 @@ protected function bubble(GeneratedUrl $generated_url, array $options = []) {
+    $options['absolute'] = is_bool($reference_type) ? $reference_type : $reference_type === self::ABSOLUTE_URL;

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -272,8 +272,8 @@ protected function getInternalPathFromRoute($name, SymfonyRoute $route, $paramet
+    $options['absolute'] = is_bool($reference_type) ? $reference_type : $reference_type === self::ABSOLUTE_URL;

Mh, so this fixes the case of ABSOLUTE_URL/ABSOLUTE_PATH but should we not also deal with the case of it being a string, see

        if (is_bool($referenceType) || is_string($referenceType)) {
            @trigger_error('The hardcoded value you are using for the $referenceType argument of the '.__CLASS__.'::generate method is deprecated since version 2.8 and will not be supported anymore in 3.0. Use the constants defined in the UrlGeneratorInterface instead.', E_USER_DEPRECATED);

            if (true === $referenceType) {
                $referenceType = self::ABSOLUTE_URL;
            } elseif (false === $referenceType) {
                $referenceType = self::ABSOLUTE_PATH;
            } elseif ('relative' === $referenceType) {
                $referenceType = self::RELATIVE_PATH;
            } elseif ('network' === $referenceType) {
                $referenceType = self::NETWORK_PATH;
            }
        }

Given that do we also expand the test coverage to use the other 2 reference types (relative path and network path)?

Hmm well we only ever supported ABSOLUTE_URL/ABSOLUTE_PATH with the previous method signature via the $absolute = FALSE bool, so no-one should ever have been passing a string in there. Since this is just a bc layer I don't think we should support things we didn't previously by it.

Don't know about RELATIVE_PATH and NETWORK_PATH - again we didn't previously support them, I guess we could add coverage to ensure that now they are supported they actually work though.

Okay that is a fair point. Can we open a follow up, given that we implement that interface, which documents it exists. People might expand it works

Opened #2676288: Symfony 2.8 supports RELATIVE_URL and NETWORK_URL in the URL generator - add test coverage.

+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -272,8 +272,8 @@ protected function getInternalPathFromRoute($name, SymfonyRoute $route, $paramet
+  public function generate($name, $parameters = array(), $reference_type = self::ABSOLUTE_PATH) {

I'm not sure if coding standards allow it, but it would be good to document the @deprecated parameters for $reference_type/$absolute here.

I'm not sure if coding standards allow it, but it would be good to document the @deprecated parameters for $reference_type/$absolute here.

IMHO we should also keep the snake case of the interface, especially because otherwise API module might have a problem with scanning it.

Includes suggestioins from #17 and #16:

we should also keep the snake case of the interface, especially because otherwise API module might have a problem with scanning it.

Indeed, the current generated docs do not detect the interface inheritance for the generate method.

I'm not sure if coding standards allow it, but it would be good to document the @deprecated parameters for $reference_type/$absolute here.

There seems to be no way to deprecate parameters in docblock so I used an if/else block and included a trigger_error. It makes for a bigger patch but it makes sense to rise a notice. I have found a few usages of trigger_error in core so I guess it is fine according to CS.

Just realized that trigger_error makes phpunit fail. I see that other core components use the @trigger_error and passes with phpunit. Attached.

I personally wouldn't use trigger_error() yet, this is Symfony's deprecation not ours yet - we can just write that the bool is deprecated in the @param docs for now. I'd like to start using E_USER_DEPRECATED a lot more in 8.2.x though so good for a follow-up. See #2575081: [policy, no patch] Use E_USER_DEPRECATED in Drupal 8 minor releases for more discussion of that.

OK, this patch is based on #12 and renames the argument to $referenceType in order to match the interface signature.

As per #1994890: Allow {@inheritdoc} and additional documentation there seems to be no way to add extra information to @inheritdoc so I did nothing regarding the deprecation info.

Can we add a follow up to actually support the other reference types as well, both in generate() and in generateFromRoute() ? For now this patch resolves the problem with the BC break.

IMHO its fine to stick with the current situation in terms of the documentation. generate() is not recommended API anyway.

You are right!

Committed c4145ed and pushed to 8.1.x and 8.2.x. Thanks!