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').
| Comment | File | Size | Author |
|---|---|---|---|
| SCR-2024-02-29.jpg | 339.45 KB | glynster |
Issue fork lupus_decoupled-3425016
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
Comment #3
roderikThanks. The first commit is a test, which is now failing. Hopefully a fix follows soon.
Comment #4
roderikI 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.
Comment #6
roderikThe MR introduces a new URL option 'lupus_decoupled_skip_rewrite'. I want to state clearly why (even if only for my future self):
BaseUrlProvider::getAdminBaseUrl(), which must not be rewritten.$option['entity']set, but we also need to rewrite the following, for which it is practically impossible to always set$option['entity']:There is another solution that tiptoes around the above and passes all our current tests** + also fixes the "View API Output" links:
As in:
isset($options['entity']); this is equal to the 2023 situation.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 ofBaseUrlProvider).For comparison, the current proposed code change is:
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
** I haven't checked our internal test suite with this yet.
Comment #7
roderikComment #8
roderikComment #9
roderikComment #10
roderikComment #11
fagothx. 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 ?
Comment #12
fagoNote: This is related: https://www.drupal.org/project/lupus_decoupled/issues/3346436
Comment #13
roderikI 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.Comment #15
fago> 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!