Over in #2737729: Canonical and AMPHTML must be absolute we changed amphtml links to be absolute rather than relative. Now we need to update the test in tests/src/Functional/AmpFormatterTest.php.

CommentFileSizeAuthor
#6 interdiff-2741217-6.txt1.17 KBmtift
#6 amp-test-absolute-urls-2741217-6.patch1.87 KBmtift
#3 amp-test-absolute-urls-2741217-2.patch1.81 KBmtift
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mtift created an issue. See original summary.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift commented
Status: Active » Needs review

This feels very hacky to me, but I couldn't figure out a better way to test for

link rel="amphtml" href="http://amp.dev/node/1?amp"

rather than

link rel="amphtml" href="http://amp.dev/node/1?amp=1"

without resorting to doing this (or some other string manipulation):

$amp_node_url = substr($amp_node_url, 0, -2);

I'm certainly open to other suggestions. But this works and the tests pass (again).

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift commented
FileSize
amp-test-absolute-urls-2741217-2.patch1.81 KB
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

It's not pretty either, but why don't you just use the same hack as when we generate the url in the code? generate it without the query, then append it. We know that there's nothing else in the test anyway..

But I'll ask @dawehner about this.

I noticed a similar same problem with e.g. redirect.module redirects (when aliases change..) then the resulting url is ?amp= and amp validator is unhappy about the difference between actual url and amphtml link on the canonical page...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Just use => NULL:

>>> \Drupal\Component\Utility\UrlHelper::buildQuery(['foo'])
=> "0=foo"
>>> \Drupal\Component\Utility\UrlHelper::buildQuery(['foo' => NULL])
=> "foo"
>>> \Drupal\Component\Utility\UrlHelper::buildQuery(['foo' => TRUE])
=> "foo=1"
>>>
mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift commented
FileSize
amp-test-absolute-urls-2741217-6.patch1.87 KB
interdiff-2741217-6.txt1.17 KB

Thanks for your input, @dawehner. However, if I use NULL, like this:

-    $amp_node_url = Url::fromRoute('entity.node.canonical', ['node' => $node->id()], ['absolute' => TRUE, 'query' => ['amp' => TRUE]])->toString();
-    // We need the URL to end in "?amp" rather than "?amp=1".
-    $amp_node_url = substr($amp_node_url, 0, -2);
+    $amp_node_url = Url::fromRoute('entity.node.canonical', ['node' => $node->id()], ['absolute' => TRUE, 'query' => ['amp' => NULL]])->toString();

Then it does not set any parameters and the tests fail with this message:

1) Drupal\Tests\amp\Functional\AmpFormatterTest::testAmpViewMode
Behat\Mink\Exception\ExpectationException: The string "link rel="amphtml" href="http://amp.dev/node/1"" was not found anywhere in the HTML response of the current page.

I'm fine with appending. That's probably more straightforward.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Oh yeah, that is clearly a bug of core: #2741617: The URL generator doesn't allow to use no query parameter value

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray at Lullabot commented
Status: Needs review » Postponed
Related issues: +#2741617: The URL generator doesn't allow to use no query parameter value

Good to see #2741617: The URL generator doesn't allow to use no query parameter value looks like it will make testing this more doable. Probably makes sense to postpone this fix until that lands in core. The alternative is doing something less ideal now, than leaving a @todo for after this gets in. Fine with that, too, but setting this to postponed to start. Feel free to change if you prefer mtift.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift commented
Status: Postponed » Needs review

No, I don't think this should be postponed, because without this patch our tests are breaking. I suggest we commit this code and open an follow-up.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift at Lullabot commented

Here's the follow-up: #2745187: Update tests to use use NULL query parameter value

RainbowArray’s picture

RainbowArray
she/her
Primary language English
Location Twin Cities, Minnesota
CreditAttribution: RainbowArray at Lullabot commented
Status: Needs review » Reviewed & tested by the community

Code looks good, tested the test, that seems to run fine. RTBC.

  • mtift committed 89c0976 on 8.x-1.x 
    Issue #2741217 by mtift: Test for absolute URLs
mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x-1.x. Thanks, all!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
CreditAttribution: mtift at Lullabot commented