Track absolute URLs in link fields

I moved the logic that finds an entity by a URL from the HtmlLink plugin to the EntityUsageTrackBase and refactored the HtmlLink and Link plugins.

Unfortunately I don't have the time and skills to create/modify the tests, so if anyone else could do that for me, that would be great!

I see the tests are failing. Don't have the time at the moment to review if it is because of the non-updated tests, or because something is really broken. Will check tomorrow.

1. +++ b/tests/src/FunctionalJavascript/EntityUsageJavascriptTestBase.php
   @@ -33,6 +33,11 @@ abstract class EntityUsageJavascriptTestBase extends JavascriptTestBase {
   +    $current_request = \Drupal::request();
   +    $config = \Drupal::configFactory()->getEditable('entity_usage.settings');
   +    $config->set('site_domains', [$current_request->getHttpHost()]);
   +    $config->save();
   +
   

2. +++ b/tests/src/FunctionalJavascript/IntegrationTest.php
   @@ -315,6 +315,65 @@ class IntegrationTest extends EntityUsageJavascriptTestBase {
   +    $page->fillField('field_link1[0][uri]', $node1->toUrl()->setAbsolute()->toString());
   

Couldn't test this locally, but reviewing the code, maybe the issue comes because if I recall correctly, the testbot runs the tests with a path prefix, so maybe there is a mismatch between the full URL you are saving into the field, and the one you are whitelisting in config?

Let's see if this improves anything.

OK, giving up for now.
This is how I think it should work (and passes for me), but the testbot doesn't appear to be wanting to collaborate.

Re-rolled against recent head and (hopefully) found the problem with the test.

Are you kidding me

Just realized that the failure comes in a part of the test that was already passing on d.org testbot before, so I wonder if there is indeed a regression somewhere...

Let's try again with less black magic in the regex.

Interestingly, the fact that https://www.drupal.org/project/entity_usage/issues/2952008#comment-12550349 is green demonstrates that our logic to whitelist the domain inside the test with:

+    // Whitelist the local hostname so we can test absolute URLs.
+    $current_request = \Drupal::request();
+    $config = \Drupal::configFactory()->getEditable('entity_usage.settings');
+    $config->set('site_domains', [$current_request->getHttpHost() . $current_request->getBasePath()]);
+    $config->save();

works, so there must be indeed a bug in ::findEntityByUrl() when used in Link fields, in some scenarios (probably when the installation uses a subdirectory?)

So we definitely need to fix that before committing this.

Reroll after #2952008: Automated tests: Refactor existing and add missing coverage was committed.

Removed these conflicting lines as these were already changed in the related issue above:

+++ b/tests/src/FunctionalJavascript/IntegrationTest.php
@@ -313,8 +313,8 @@ class IntegrationTest extends EntityUsageJavascriptTestBase {
     $session->wait(500);
-    $assert_session->pageTextContains('eu_test_ct Node 1 has been created.');
     $this->saveHtmlOutput();
+    $assert_session->pageTextContains('eu_test_ct Node 1 has been created.');
     /** @var \Drupal\node\NodeInterface $node1 */

@@ -351,8 +351,8 @@ class IntegrationTest extends EntityUsageJavascriptTestBase {
     $session->wait(500);
-    $assert_session->pageTextContains('eu_test_ct Node 2 has been updated.');
     $this->saveHtmlOutput();
+    $assert_session->pageTextContains('eu_test_ct Node 2 has been updated.');
     // Verify the usage was released.

+++ b/src/EntityUsageTrackBase.php
@@ -296,14 +296,16 @@ abstract class EntityUsageTrackBase extends PluginBase implements EntityUsageTra
+      if (isset($matches[1]) && $target_entity_type = $this->entityTypeManager->getDefinition($matches[1])) {

The isset($matches[1]) is unnecessary since the preg_match() will return 0 (false) when no match is found. With the current pattern you can assume $matches[0-2] are always set inside this if statement.

Rerolled #34.

#3060687: Support Layout Builder relationships landed recently, it needs to update the __construct signature accordingly.

The tests run fine locally, why failed on D.O.

@weynhamz thanks for your contribution!

I haven't reviewed the patch, but see #26 and #28 for more info. At that time, there seemed to be legitimate concerns about the failures on d.o indicating real issues with the logic being introduced here.

It would be helpful if you could provide interdiffs between patches, it makes it easier to identify what changed between two versions of the same patch. You can find more information about creating such a file here. Thanks!

@marcoscano Thanks for point out internal diff, I will provide one for the final patch.

Meanwhile, I am just submitting a patch that just moved the code around without introducing any new changes, let's see how the test bot says about it.

Finally, manage to find why the `testLinkTracking()` is failing on test bot.

I printed out the $url = $link->getUrl()->toString(); in \Drupal\entity_usage\Plugin\EntityUsage\Track\Link::getTargetEntities, and the path is has a `/subdirectory` prefix, which later $this->pathValidator->getUrlIfValidWithoutAccessCheck($url) would not be able to handle.

https://dispatcher.drupalci.org/job/drupal8_contrib_patches/17534/artifact/jenkins-drupal8_contrib_patches-17534/artifacts/run_tests.js/simpletest_html/Drupal_Tests_entity_usage_FunctionalJavascript_IntegrationTest-32-90569414.html

Status message
/subdirectory/node/1 |||
Failed to get url object |||
Can not get a valid entity |||
eu_test_ct Node 2 has been created.

I miss my bed now, continue on this tomorrow.

Since `$link->getUrl()->toString()` will return a path with `subdirectory` prefix because of D.O. testbot runs in the subdirectory, and we only what to handle the situation that `$link` is actually an external absolute link, let's handle things differently then.

I will provide an interdiff if testbot is happy with this patch.

It shall pass.

Patch in #40 can be considered as a reroll of #24, without any new change introduced later in #26, #29, #31, #33, #34.

Update interdiff between #40 and #45 that we know why testbot is happy with #45.

Add back new changes introduced in #26, #29, #31, #33, #34.

Expect test failing.

Add back new changes introduced in #26, #29, #31, #33, #34.

Also, add back below code that got removed in #34.

+          if ($element->hasAttribute('data-entity-uuid')) {
+            // Normally the Linkit plugin handles when a element has this
+            // attribute, but sometimes users may change the HREF manually and
+            // leave behind the wrong UUID.
+            $data_uuid = $element->getAttribute('data-entity-uuid');
+            // If the UUID is the same as found in HREF, then skip it because
+            // it's LinkIt's job to register this usage.
+            if ($data_uuid == $entity->uuid()) {
+              continue;
+          }

Fix a stupid mistake in #47.

Now it shall pass the tests.

Let's introduce these new changes in a safe way, step by step.

I am using #45 as baseline now, add interdiff of 45-49.

Introduce changes in #31 and #33, but keep the original entity match pattern.

I think the proposal in #33 about use `entity.*.canonical` route pattern would be better to handle and discuss in a separate issue because we might need to have more flexibility about the entity pattern. For example, I have a case where I am using media_entity_download and the entity pattern for that is media_entity_download.download, maybe, have it configurable somehow?

Fix stupid error in the last patch.

Add changes in #26.

https://dispatcher.drupalci.org/job/drupal8_contrib_patches/17724/artifact/jenkins-drupal8_contrib_patches-17724/artifacts/run_tests.js/simpletest_html/Drupal_Tests_entity_usage_FunctionalJavascript_IntegrationTest-38-49315841.html

1. php-apache-jenkins-drupal8-contrib-patches-17724/subdirectory
2. php-apache-jenkins-drupal8-contrib-patches-17724/subdirectory/
3. /php-apache-jenkins-drupal8-contrib-patches-17724\/subdirectory\//
4. /subdirectory/node/1
5. /subdirectory/node/1
eu_test_ct Node 3 has been created.

Domain handling in the new code introduced in #26 does not get along with the testbot subdirectory setup.

Looks like the domain used by testbot is `php-apache-jenkins-drupal8-contrib-patches-17724/subdirectory`, we need to to support subdirectory suffixed domain.

Fixed testbot failure in #53.

The only thing that is not included in this patch, is the proposal in #33 about use `entity.*.canonical` route pattern, which I think should be in a separate issue to discuss.

We have a few options now.

#45: this patch just tried to move the logic to handle HTML link to the base abstract class and make the Link plugin support absolute URL as well, does not introduce any other new code changes, just as this issue's original purpose.

#49: this patch can be think as a reroll of #5, with some safe code improvements.

#52: this patch added back the changes in #31 and #33, but left behind the `entity.*.canonical` route pattern change.

And finally, this one added back the regex pattern change in #26.

Of course, testbot is happy about all of these.

Improvement for EntityUsage type hint to EntityUsageInterface

Rerolled for beta7.

Fixed a copy/paste error.

Sorry all for my late response on this. Committed to 8.x-2.x.
Thanks all for contributing!