Closed (fixed)
Project:
Lupus Decoupled Drupal
Version:
1.0.0-beta6
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
11 Mar 2024 at 09:56 UTC
Updated:
16 Apr 2024 at 19:54 UTC
Jump to comment: Most recent
Comments
Comment #2
roderikComment #4
perraudeau commentedHello,
I've encountered the same issue when using Lupus in a decoupled configuration along with the Domains and Domain module. To address this, I've created a fork branch that resolves the problem in my specific scenario, while aiming to maintain as much generality as possible.
I believe this patch could help to this issue.
Comment #5
roderik@perraudeau thank you for your contribution!
I've looked at your code, and my conclusion is that I'm still going to create a PR with the proposed resolution: change getAdminBaseUrl(). Because:
This still needs internal testing from our side, and a fix for a strange issue, so it's not going to get merged yet.
--
In the meantime, I'm curious to your issue:
Then we can use this for PHPUnit tests, which should be added before a solution gets committed. (If you can contribute a precise description for an example setup with domain module, if not PHPUnit tests... that helps.)
---
And then, we can also discuss your solution. It may not be necessary to go into detail right now, but I'll still describe my review of your code. I've seen:
$entity->toUrl()withUrl::fromRoute(). I don't have a fundamental problem with that except.../ce-api/node/{node}. I am hoping that this won't be necessary in the end. (Maybe we can get$entity->toUrl()to work in #3346436: Introduce API for getting the /ce-api URL, and make sure it works with the Domain module at the same time?)DOMAIN/LANGCODE/ce-api/node/..., which are not supported without your patch (we only supportDOMAIN/ce-api/LANGCODE/node/.... We can discuss this if it's really necessary, but I am hoping we can do without this. At any rate, thestr_contains()change in BackendApiRequest.php is too broad. We do not want to match + change /ce-api/ just anywhere in the path.I have tried the tests I wrote, with your solution. They run fine. (Except they fail on
DOMAIN/ce-api/...changes to/ce-api/....) I don't think that's a problem and we could change the tests if needed.)
--
The "strange issue" which is still failing the tests:
I have enabled the views module in PHPUnit tests, because without views I don't get an original + translated node in the same list, in /admin/content.
But now... a separate test is failing: the json-ld-schema breadcrumbs on /node/1 seem to be containing the backend URL, when views is enabled!
Even though that's a separate issue, we should still fix it and we can't commit something with failing tests. I'll look into it later.
Comment #7
roderikComment #8
roderikMR #82 works. It's a simple fix... combined with a lot of test additions / workarounds. For review now.
@perraudeau if this gets set to "Fixed" after review, and your issue with the domain module persists, feel free to open a new ticket. As said: I'd love to know which "wrong" links are fixed by your change, and what the "right / fixed" links are.
Comment #10
fagook, looks good now. Also tested successfully, thus merged. Thank you!