Problem/Motivation

When e.g. viewing the content admin screen at /de/admin/content, the "View API Output" links in the actions-dropdown-buttons of each row are invalid.

Proposed resolution

More fundamentally: BaseUrlProvider::getAdminBaseUrl() returns the language prefix and shouldn't. That's a behavior change but we can still fix it in beta.

When that's done, we should reevaluate / the bug will probably be gone.

API changes

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

roderik created an issue. See original summary.

roderik’s picture

Issue summary: View changes

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

perraudeau’s picture

Hello,

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.

roderik’s picture

@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:

  • For the basic case (without domain module), I believe the issue description is enough. It's simpler.
  • The fact that getAdminBaseUrl() has a language prefix, is unintended behavior that is causing problems elsewhere. So it should be fixed anyway.

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:

  • Can you test whether the new PR resolves your issue? (I'm guessing it won't, because the domain module adds complications)
  • If not: can you describe precisely
    • what API urls you see, without your patch (and where - if not in /admin/content
    • and what they should be?

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:

  • Fundamentally you're replacing $entity->toUrl() with Url::fromRoute(). I don't have a fundamental problem with that except...
  • You're adding a route for /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?)
  • With this, you are adding support of URLs like DOMAIN/LANGCODE/ce-api/node/..., which are not supported without your patch (we only support DOMAIN/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, the str_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

  1. links are with the backend domain before your patch, and without the backend domain after. (So: DOMAIN/ce-api/... changes to /ce-api/....) I don't think that's a problem and we could change the tests if needed.
  2. the same strange/separate issue as with my solution

)
--

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.

roderik’s picture

Issue summary: View changes
roderik’s picture

Status: Active » Needs review

MR #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.

  • fago committed 518410bc on 1.x authored by roderik
    Issue #3427027 by roderik: Fix "View API Output" links wrong when viewed...
fago’s picture

Status: Needs review » Fixed

ok, looks good now. Also tested successfully, thus merged. Thank you!

Status: Fixed » Closed (fixed)

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