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.
CommentFileSizeAuthor
#10 drupal-1893730-9.patch20.88 KBdawehner
#7 drupal-1893730-7.patch10.54 KBdawehner
#7 interdiff.txt3.28 KBdawehner
#3 drupal-1893730-3.patch9.29 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

This sounds related to #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Assigned: Unassigned » dawehner

Assigning.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Active » Needs review
FileSize
drupal-1893730-3.patch9.29 KB

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

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
+++ 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.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

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.

Eric_A’s picture

Eric_A CreditAttribution: Eric_A commented

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.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
interdiff.txt3.28 KB
drupal-1893730-7.patch10.54 KB

Thanks for the review!

* Finger crossing *

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

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.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
drupal-1893730-9.patch20.88 KB

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.

Crell’s picture

Crell CreditAttribution: Crell commented

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

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

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

Crell’s picture

Crell CreditAttribution: Crell commented

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.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

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.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Assigned: dawehner » Unassigned

. The approach in the tests seems wrong

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
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.

Crell’s picture

Crell CreditAttribution: Crell as a volunteer commented
Issue summary: View changes
Status: Needs work » Closed (won't fix)

I'm going to say this is sufficiently stale as to not be relevant anymore.