Drupal\Core\Routing\RouteCompiler::compile($route) assumes that the '$route' object is injected.

Patch still applies to 9.1.x.

Thank for the issue and patch. I understand the changes in the code but I wonder if this has to be changed in 10.0.x since the constructor signature would change so anything extending CompiledRoute would break unless they updated their signature. I'll check with the #bugsmash team.

Added a quick test demonstrating the issue.

So the docs for CompiledRoute say

/**
 * A compiled route contains derived information from a route object.
 */

This seems to imply that it is a simple value object that should not contain a reference to the original Route object itself.

Where are the getOptions, getDefaults and getRequirements methods useful? To create a CompiledRoute you need the original Route object anyway, so you can just call the methods on the original object? Should we just deprecate and remove these methods instead, as they can never work as it stands?

The change suggested here is effectively undoing #2381505: Unserialize preloaded routes on the fly.

It looks like that issue should really have removed getOptions, getDefaults and getRequirements as well. The more I think about it the more I think we should just remove these methods now, as they have not worked since that issue above was committed in 2015, and so why even deprecate them?

@longwave: I think you are right, and the issue you linked to did remove other methods tied to the $route property (getPath() and getRoute()). Removing getOptions(), getDefaults() and getRequirements() seems to be the right approach.

@longwave and @mr.baileys Thanks for the updates and analysis. Makes sense to me. Tagging for Global2020 so maybe someone will grab this as it's pretty straightforward. :)

I'm trying to replicate this locally, but the steps aren't really clear to me. @mr.baileys, were you able to replicate this? If so, how? I do have 9.1 installed locally.

@partyka I was able to reproduce in Drupal 9 using the steps that I've added to the issue summary.

Form error: Call to a member function getDefaults() on null

Log error:

Notice: Undefined property: Drupal\Core\Routing\CompiledRoute::$route in Drupal\Core\Routing\CompiledRoute->getDefaults() (line 130 of /var/lib/tugboat/stm/web/core/lib/Drupal/Core/Routing/CompiledRoute.php)

Though from #12 and #13, the approach seems to be:

Removing getOptions(), getDefaults() and getRequirements() seems to be the right approach.

so this issue summary could be rewritten for this focus.

I am not sure if we can get away with just removing those functions (since they haven't worked for a long while now), or if they need proper deprecations.

Briefly discussed this with @lendude on #bugsmash, who said:

My 2cts would be that we do need to deprecate them, because just removing them would take this from a Notice to a Fatal error. So the ‘Safe way’ would be the deprecation route with longwave’s #11 comment probably as the basis for a replacement message ‘call these methods on the original Route object’ or something along those lines.

So let's deprecate those functions instead of removing them.

Here's a patch that:

1) Marks the methods as deprecated.
2) Removes the calls on the null object as it will never work (Note: I inspected the class tree and did not find anywhere that the `route` object was a property.
3) Adds tests for the deprecation.

Since this will be marked as removed in 10.0.0, does this also need to be removed in the 10 branch?

Regarding the deprecation notice... I'm unsure as to what a decent notice should be. The steps to reproduce use a chained call, so the deprecation notice can't really say "call on the original object".

Doh... #10 is a bad patch. here's a better one.

Thanks for the patch! This is looking good and I think the "no direct replacement" message is OK, as these never would have worked properly anyway. A few minor comments:

1. +++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
   @@ -113,11 +113,17 @@ public function getPatternOutline() {
   +   * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. No direct
   +   *   replacement is provided.
   

   This should be added to the getDefaults and getRequirements methods as well.

2. +++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
   @@ -113,11 +113,17 @@ public function getPatternOutline() {
   +   * @see https://www.drupal.org/node/3150727
   +   * @see \Drupal\Core\Extension\ExtensionList::checkIncompatibility()
   

   This doesn't seem relevant to this patch.

3. +++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
   @@ -125,9 +131,10 @@ public function getOptions() {
   +   *
   

   Extra blank line can be removed.

4. +++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
   @@ -137,7 +144,7 @@ public function getDefaults() {
   +   @trigger_error(__METHOD__ . '() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. No direct replacement is provided. See https://www.drupal.org/node/3159706', E_USER_DEPRECATED);
   

   This line is indented incorrectly.

New patch with the minor details fixed.

I've also corrected the node the @see statement is pointing to so that it points to the change record, and I've also updated the change record. -- https://www.drupal.org/node/3159706

Thanks for pointing out #2, you're correct it's irrelevant ... bad copy and paste. 🤦🏻‍♂️

Thanks for updating the patch. This is ready for RTBC but I have one question that I don't know the answer to:

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
@@ -113,31 +113,46 @@ public function getPatternOutline() {
-    return $this->route->getOptions();
...
-    return $this->route->getDefaults();
...
-    return $this->route->getRequirements();

I wonder if we should leave these broken return statements in place.

There is a chance (admittedly very small) that someone is doing

$r = new CompiledRoute(...);
$r->route = $route;
...
$r->getOptions();

which would actually work right now, but is broken by this patch.

So, if leave the broken return statements in place, the tests won't pass (locally, at least). Here's what happens when I run these tests locally (output slightly altered for clarity):

cd core; ../vendor/bin/phpunit tests/Drupal/Tests/Core/Routing/CompiledRouteLegacyTest.php
PHPUnit 8.5.8 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\Routing\CompiledRouteLegacyTest
EEE                                                                 3 / 3 (100%)

Time: 369 ms, Memory: 6.00 MB

There were 3 errors:

1) Drupal\Tests\Core\Routing\CompiledRouteLegacyTest::testOptionsDeprecated
Undefined property: Drupal\Core\Routing\CompiledRoute::$route

~/Sites/drupal/9.1/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php:212
~/Sites/drupal/9.1/core/lib/Drupal/Core/Routing/CompiledRoute.php:126
~/Sites/drupal/9.1/core/tests/Drupal/Tests/Core/Routing/CompiledRouteLegacyTest.php:29
~/Sites/drupal/9.1/vendor/phpunit/phpunit/src/Framework/TestResult.php:691
~/Sites/drupal/9.1/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
~/Sites/drupal/9.1/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:627
~/Sites/drupal/9.1/vendor/phpunit/phpunit/src/TextUI/Command.php:204
~/Sites/drupal/9.1/vendor/phpunit/phpunit/src/TextUI/Command.php:163

2) Drupal\Tests\Core\Routing\CompiledRouteLegacyTest::testDefaultsDeprecated
Undefined property: Drupal\Core\Routing\CompiledRoute::$route

~/Sites/drupal/9.1/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php:212
~/Sites/drupal/9.1/core/lib/Drupal/Core/Routing/CompiledRoute.php:142
~/Sites/drupal/9.1/core/tests/Drupal/Tests/Core/Routing/CompiledRouteLegacyTest.php:37
~/Sites/drupal/9.1/vendor/phpunit/phpunit/src/Framework/TestResult.php:691
~/Sites/drupal/9.1/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
~/Sites/drupal/9.1/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:627
~/Sites/drupal/9.1/vendor/phpunit/phpunit/src/TextUI/Command.php:204
~/Sites/drupal/9.1/vendor/phpunit/phpunit/src/TextUI/Command.php:163

3) Drupal\Tests\Core\Routing\CompiledRouteLegacyTest::testRequirementsDeprecated
Undefined property: Drupal\Core\Routing\CompiledRoute::$route

~/Sites/drupal/9.1/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php:212
~/Sites/drupal/9.1/core/lib/Drupal/Core/Routing/CompiledRoute.php:158
~/Sites/drupal/9.1/core/tests/Drupal/Tests/Core/Routing/CompiledRouteLegacyTest.php:45
~/Sites/drupal/9.1/vendor/phpunit/phpunit/src/Framework/TestResult.php:691
~/Sites/drupal/9.1/vendor/phpunit/phpunit/src/Framework/TestSuite.php:597
~/Sites/drupal/9.1/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:627
~/Sites/drupal/9.1/vendor/phpunit/phpunit/src/TextUI/Command.php:204
~/Sites/drupal/9.1/vendor/phpunit/phpunit/src/TextUI/Command.php:163

----
Testing each method using devel/php reveals the following:

getDefaults():

getOptions():

getRequirements():

----
Test with `drush php-eval`:

./vendor/bin/drush php-eval '$route = new \Symfony\Component\Routing\Route("/"); $retval = \Drupal\Core\Routing\RouteCompiler::compile($route)->getRequirements();'
./vendor/bin/drush php-eval '$route = new \Symfony\Component\Routing\Route("/"); $retval = \Drupal\Core\Routing\RouteCompiler::compile($route)->getOptions();'
./vendor/bin/drush php-eval '$route = new \Symfony\Component\Routing\Route("/"); $retval = \Drupal\Core\Routing\RouteCompiler::compile($route)->getDefaults();' 

I'm not sure how this could have ever worked. All the commands cause drush to terminate abnormally with an error like Error: Call to a member function getRequirements() on null

Yeah, the only way is if someone did something like my suggestion in #28, we could mock a Route and add that to the test?

So I added the mocked Route object in the setUp() method.

But, now that I think about this... maybe it makes more sense to add a conditional to these three methods to test if `route` has0 been set, and if so, to behave with the broken behavior. Route has no visibility, so it's conceivable that some one set it like you had.

Here's the latest patch just so you can see.

Update the patch so that we now test to see if the property has been dynamically set, and if it has been, to check to see if it's a route object, and if so, to behave like how it was implied to behave before. Otherwise, it will just return an empty string.

Changed from using prophesize to use the `createMock` method.

Removed unused `use` statements.

I've noticed the code style issues, but before I create another patch, I have some questions about what's a good way to adjust the code so that the `\Drupal\Core\Routing\RouteCompiler::compile($route)->getDefaults();` methods don't produce errors, as well as handling the possibility that someone could have dynamically set the `route` variable:

I’m really uncertain what’s a good way to suppress error messages generated … I’m currently checking to see if it’s a Route object, but that might be too specific. I might be able to see if the method is set on the object by using Method Exists. That might be enough since the method takes no parameters. Thoughts?

---
Also, apologies for all the patches for the code style errors. I have PhpStorm set up for my website projects, but I didn't do that for core.

Just a generic review

1. 6 coding standards messages
   /var/lib/drupalci/workspace/jenkins-drupal_patches-53973/source/core/tests/Drupal/Tests/Core/Routing/CompiledRouteLegacyTest.php
   line 6 There must be one blank line after the last USE statement; 2 found;
   68 Expected 1 blank line after function; 2 found
   78 Empty PHP statement detected: superfluous semi-colon.
   90 Expected 1 blank line after function; 2 found
   110 Expected 1 blank line after function; 2 found
   113 The closing brace for the class must have an empty line before it

   See https://www.drupal.org/pift-ci-job/1762580

2. +++ b/core/tests/Drupal/Tests/Core/Routing/CompiledRouteLegacyTest.php
   @@ -0,0 +1,113 @@
   +   * @var \Drupal\Core\Routing\CompiledRoute
   +   *   A compiled route object for testing purposes.
   ...
   +   * @var \Symfony\Component\Routing\Route
   +   *   A mocked Route object.
   

   Should move the comment to above the @var line as they are properties, for example:

   /** 
     * A compiled route object for testing purposes.
     *
     * @var \Drupal\Core\Routing\CompiledRoute
     */

3. +++ b/core/tests/Drupal/Tests/Core/Routing/CompiledRouteLegacyTest.php
   @@ -0,0 +1,113 @@
   +  protected function setUp(): void {
   

   Missing {@inheritdoc}

4. +++ b/core/tests/Drupal/Tests/Core/Routing/CompiledRouteLegacyTest.php
   @@ -0,0 +1,113 @@
   +   * Test for deprecrated message and no PHP error.
   ...
   +   * Test to make sure we get an array when dynamically setting.
   ...
   +   * Test for deprecrated message and no PHP error.
   ...
   +   * Test to make sure we get an array when dynamically setting.
   ...
   +   * Test to make sure we get an array when dynamically setting.
   

   Should start with verb, 'Test' -> 'Tests'

@partyka Thanks for the updates. I didn't properly review but did notice some formatting issues like extra blank lines. It is probably worth running things through the code sniffer for these.

Also, please always include an interdiff when you change patches so it's easier to understand what has changed and what has not changed: https://www.drupal.org/documentation/git/interdiff

Thanks.

UPDATE: I see I crossposted with @jungle and he gave a proper review. :)

Working on this.

Review please, thanks.

1. +++ b/core/tests/Drupal/Tests/Core/Routing/CompiledRouteLegacyTest.php
   @@ -15,8 +15,9 @@
   +   * ¶
   
   @@ -25,7 +26,10 @@ class CompiledRouteLegacyTest extends UnitTestCase {
   +  ¶
   

   Unexpected whitespaces

2. +++ b/core/tests/Drupal/Tests/Core/Routing/CompiledRouteLegacyTest.php
   @@ -68,19 +72,18 @@ public function testOptionsDynamicallySet() {
   -    $this->assertIsArray($this->compiled_route->getDefaults());;
   

   Oh, double ;, nice catch!

Back to needs work to fix whitespace.

Apology, review please, thanks.

Thanks for the update. The interdiff looks good but leaving status for additional review and testing.

@Kristen Pol -- agreed about the code style, and apologies for that. I was waiting for a review on how to handle the changes to CompiledRoute so that we don't accidentally cause a BC break for someone (or agree that it's an acceptable BC break).

Generated new patch to remove unused "use" statements, as well as generated an interdiff this time. :-)

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
@@ -113,31 +114,61 @@ public function getPatternOutline() {
-    return $this->route->getOptions();
+    @trigger_error(__METHOD__ . '() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. No direct replacement is provided. See https://www.drupal.org/node/3159706', E_USER_DEPRECATED);
+    if (isset($this->route) && $this->route instanceof Route) {
+      return $this->route->getOptions();
+    }
+    // Returning an array to provide a consistent return value with the phpdoc.
+    return [];

I am not sure we need to do any of this. This issue is a task to deprecate these methods, as they haven't worked for some time. If we intend to fix them or alter their behaviour in any way it should be done in a separate issue (and even then, I don't think that is necessary, as we are just going to remove them in the future). I think this method and the other similar ones should just read:

    @trigger_error(__METHOD__ . '() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. No direct replacement is provided. See https://www.drupal.org/node/3159706', E_USER_DEPRECATED);
    return $this->route->getOptions();

@longwave, I agree with you about it being un-necesssary. Attached is a new patch + interdiff.

Couple of minor coding standards issues left, but this looks good to me now!

1. +++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
   @@ -3,6 +3,7 @@
   +use Symfony\Component\Routing\Route;
   

   Unused use.

2. +++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
   @@ -113,30 +114,49 @@ public function getPatternOutline() {
   +
   

   Extra blank line.

Thanks @longwave for the review.

Addressing #48

Thanks! I almost think we have too much test coverage now, but I guess these weren't tested before and it will all be removed together when Drupal 10 opens so it can't hurt.

Random fail in CKEditorIntegrationTest, back to RTBC

Committed 192e9df and pushed to 9.1.x. Thanks!

(fixed a 'deprecrated' typo on commit).

Publish the change record