Problem

  • Any call to url() requires an installed, working, and correct database schema of {url_alias}.

Goal

  • Provide a in-memory/mock implementation of Path\AliasManager to remove the dependency on {url_alias} when it is not needed.
Files: 
CommentFileSizeAuthor
#10 drupal-1893730-9.patch20.88 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1893730-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#7 drupal-1893730-7.patch10.54 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 50,470 pass(es), 2 fail(s), and 4 exception(s).
[ View ]
#7 interdiff.txt3.28 KBdawehner
#3 drupal-1893730-3.patch9.29 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 50,766 pass(es).
[ View ]

Comments

Assigned:Unassigned» dawehner

Assigning.

Status:Active» Needs review
StatusFileSize
new9.29 KB
PASSED: [[SimpleTest]]: [MySQL] 50,766 pass(es).
[ View ]

That's just a first step, let's see how many of the unit tests break with that.

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -156,6 +156,8 @@ public function build(ContainerBuilder $container) {
+    $container->register('path.alias_manager.memory', 'Drupal\Core\Path\MemoryAliasManager');

I think this should just be registered when needed, like the other in memory implementations.

This patch should allow us to remove all

    $this->installSchema('system', 'url_alias');

from all DUTB tests; e.g., one of them is Drupal\filter\Tests\FilterDefaultConfigTest.

If possible, we should do that as part of this issue + patch.

FYI: This test and others are postponed. HEAD seems to be broken. So far no issue seems to have been identified as the source of the problem.

StatusFileSize
new3.28 KB
new10.54 KB
FAILED: [[SimpleTest]]: [MySQL] 50,470 pass(es), 2 fail(s), and 4 exception(s).
[ View ]

Thanks for the review!

* Finger crossing *

Hm. Technically, Path\AliasTest should be rewritten so that it can be re-used for both implementations. That is, because we expect both implementations to work identically. An example for that can be found in Config\Storage\ tests (DUTB), but also in Cache backend tests (unit). In essence: An abstract base test class that contains the actual test methods, which is getting extended by backend-specific test classes, which each contain a set of helper methods to access the raw storage (typically CRUD methods).

Second, I'd think that we can simply replace the service definition of path.alias_manager in DUTB::containerBuild()? We're doing the same for config.storage already. (I'm not sure why that special-casing for keyvalue exists and need to analyze that, but that's a separate issue) — Anyway, if any DUTB test needs the regular implementation for whatever reason, then it can override it back.

Status:Needs review» Needs work

The last submitted patch, drupal-1893730-7.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new20.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1893730-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

It seems to be that using the Path object with another alias manager is the total wrong approach

Status:Needs review» Needs work

The last submitted patch, drupal-1893730-9.patch, failed testing.

Er. Unless I'm misunderstanding something here we already have one. I added it as part of the CMF Routing patch so that I could unit test our generator. See Drupal\system\Tests\Routing\MockAliasManager.

It could probably be moved elsewhere to give it higher profile, but it works for unit testing. (See the other Mock classes in the same directory, too.)

If you compare the code at least MockAliasManager would fail hard on those tests.

Perhaps; it was a quick-job to help test something else. Either way all we need is a simple mock object, and only one of them. So we should if nothing else have only one very-easy-to-configure-in-a-unit-test class at the end.

So i'm wondering whether the testLookupPath method should always just use the alias manager and not \Drupal\Core\Path\Path as that's trying to write to the database all the time.

Assigned:dawehner» Unassigned

. The approach in the tests seems wrong

Status:Needs work» Needs review
Issue tags:-Framework Initiative, -API clean-up, -Testing system

#10: drupal-1893730-9.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, drupal-1893730-9.patch, failed testing.