After upgrading to 1.0.0-beta5 all View API Output links are pointing to the nuxt api instead of Drupal.

Referencing issue:
https://www.drupal.org/project/lupus_decoupled/issues/3414438#comment-15...

Analysis done while testing a fix

First:

An underlying issue: BaseUrlProvider::getAdminBaseUrl() (and therefore BaseUrlProvider::getApiBaseUrl()) gets rewritten to a frontend url now! (I missed that and none of our existing tests failed.)

We need to introduce some check to prevent this. I added a new URL option 'lupus_decoupled_skip_rewrite', which seems the cleanest fix to me, even though I don't really like introducing a special option. (There is an alternative fix to this issue: see comment #6 for all details.)

Then:

Based on that change, we need to fix the "View API output" links. We could do that

  • by setting $options['lupus_decoupled_skip_rewrite'] when generating those links,
  • or by never rewriting base URLs when $options['base_url'] is set already.

To me, it feels clearer to do the second thing. In my feeling/estimate it would be good to try to contain usage of $options['lupus_decoupled_skip_rewrite'] to just BaseUrlProvider::getAdminBaseUrl(), and the general $options['base_url'] check feels potentially useful for other future code that sets it.

I don't have proof for that feeling. But... now that we've broken things already anyway in 1.0.0-beta5, and now that we have tests for all situations we know to be significant... I feel comfortable introducing that extra base_url check. And I fail to think of a situation where code explicitly sets $options['base_url'] for a frontend path, and that path must still be subject to automatic rewriting.

Solution

The above leads to the following code change in LupusFrontendPathProcessor:

-    if (isset($options['route']) && in_array($options['route']->getPath(), $this->frontendPaths) && $request) {
+    if (isset($options['route']) && in_array($options['route']->getPath(), $this->frontendPaths)
+      && empty($options['lupus_decoupled_skip_rewrite']) && empty($options['base_url']) && $request) {

with 'lupus_decoupled_skip_rewrite' set only in BaseUrlProvider::getAdminBaseUrl(). (The "View API output" links already set 'base_url').

CommentFileSizeAuthor
SCR-2024-02-29.jpg339.45 KBglynster
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

glynster created an issue. See original summary.

roderik made their first commit to this issue’s fork.

roderik’s picture

Status: Active » Needs work

Thanks. The first commit is a test, which is now failing. Hopefully a fix follows soon.

roderik’s picture

I feel like I'm taking a stab in the dark / retroactively establishing 'rules' for the rewriting of URLs, to fit whatever works.

Well, a little tweak worked / at least passes all the tests: do not change the base URL if $options['base_url'] is already set.

(We must still change things if $options['base_url'] is not set yet but $options['absolute'] is set to TRUE -- otherwise many things start failing.)

I'll create a MR but won't set "Review" state before it has run through our internal test suite - and who knows what things that will trigger.

roderik’s picture

The MR introduces a new URL option 'lupus_decoupled_skip_rewrite'. I want to state clearly why (even if only for my future self):

  • In a non-API request == backend HTML page, we need* to rewrite links to frontend destinations. In practice: we need the node-view links in /admin/content to point to the frontend -- and we need FrontendRedirectSubscriber to redirect those same URLs.
  • We have no easy way to distinguish between these links, and a call to BaseUrlProvider::getAdminBaseUrl(), which must not be rewritten.

There is another solution that tiptoes around the above and passes all our current tests** + also fixes the "View API Output" links:

-    if (isset($options['route']) && in_array($options['route']->getPath(), $this->frontendPaths) && $request) {
+    if (isset($options['route']) && in_array($options['route']->getPath(), $this->frontendPaths) && empty($options['base_url']) && $request) {
       $absolute = $options['absolute'] ?? FALSE;
-      if ($absolute || !$this->isApiResponse($request)) {
+      if ($this->isApiResponse($request) ? $absolute : isset($options['entity'])) {

As in:

  • For non-API responses, only rewrite links where isset($options['entity']); this is equal to the 2023 situation.
  • For API responses, only rewrite absolute links - which means that BaseUrlProvider::getAdminBaseUrl() will still be rewritten to the frontend base URL.

Apparently we don't currently need BaseUrlProvider::getAdminBaseUrl() in API responses - but this still feels fishy to me. It feels clearer to introduce a new URL option 'lupus_decoupled_skip_rewrite' option to prevent that (hopefully to be used only inside of BaseUrlProvider).

For comparison, the current proposed code change is:

-    if (isset($options['route']) && in_array($options['route']->getPath(), $this->frontendPaths) && $request) {
+    if (isset($options['route']) && in_array($options['route']->getPath(), $this->frontendPaths)
+      && empty($options['lupus_decoupled_skip_rewrite']) && empty($options['base_url']) && $request) {

where && empty($options['base_url']) could also be left out and replaced by setting $options['lupus_decoupled_skip_rewrite'] explicitly for "View API output".

--

* I'm treating this "need" as a given: the code which rewrites links to frontend destinations has existed since Oct 2022. As far as I've been able to see, we could theoretically do without it, but that would mean

  • the frontend destination links not being visible in users' browsers in e.g. admin/content, which is arguably a UX degredation
  • extra redirects from the backend to the frontend destination, whenever such a link is clicked.
  • some code change needed in FrontendRedirectSubscriber; I haven't checked those details.

** I haven't checked our internal test suite with this yet.

roderik’s picture

Issue summary: View changes
roderik’s picture

Issue summary: View changes
roderik’s picture

Issue summary: View changes
roderik’s picture

Issue summary: View changes
fago’s picture

Status: Needs review » Needs work

thx. I think the analysis and changes make sense.

Only think I think we should improve is the naming of the option "lupus_decoupled_skip_rewrite", because without knowing this issues, the name would not tell me what it is about. What rewrite? In the end, the flag is about to force the baseURL to the backend (or frontend?).

What about supporting the flag 'lupus_decoupled_frontend' => FALSE or 'lupus_decoupled_rewrite_frontend' => FALSE ?

fago’s picture

roderik’s picture

Status: Needs work » Needs review

I have an in-grown reservation against parameters that are unset and default-TRUE ... but that doesn't matter much here because we don't actually want this one to be used much.

Implemented as 'lupus_decoupled_rewrite_frontend' => FALSE. Tests are green.

  • fago committed 872f2aee on 1.x authored by roderik
    Issue #3425016 by roderik: Fix "View api output" links to point to the...
fago’s picture

Status: Needs review » Fixed

> I have an in-grown reservation against parameters that are unset and default-TRUE ... but that doesn't matter much here because we don't actually want this one to be used much.

:D I dislike parameters which have a negation in the naming.. ;-) We can bike-shed!

Anyway, this is good to go, merged, thank you!

Status: Fixed » Closed (fixed)

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