Problem/Motivation

The BackendApiRequest middleware is looking for the ce-api prefix in the server's REQUEST_URI. This is not working for installations with base paths. For example Drupal's gitlab ci installation uses the base url http://localhost/web. The actual uri that is checked in the middleware is /web/ce-api/node/1 which the middleware does not recognize as a valid ce-api request and therefore passes the request through without altering it.

Steps to reproduce

Send a request against the ce-api on an installation with a base path.

Proposed resolution

Consider base paths in the middleware.

Remaining tasks

  • Fix middleware ✔️
  • Add a small unit-test here where we mock $request object, so we could fire it up with all kind of URI combinations and verify it's all correct ✔️

User interface changes

API changes

Data model changes

Issue fork lupus_decoupled-3409409

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:

About issue forks

Comments

arthur_lorenz created an issue. See original summary.

arthur_lorenz opened merge request !71

utor.io’s picture

utor.io commented
Assigned: utor.io » Unassigned
Issue summary: View changes
Status: Needs work » Needs review

Added a simple unit test to check if base paths are considered.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

Looks good to me, just a suggestion for

['/web/ce-api', '', NULL]
['/ce-api?param=1', '', TRUE]
['/ce-api/?param=1', '', TRUE]

I don't know how that works in your list's organization -- lots of dimensions unfortunately :)

utor.io’s picture

utor.io commented

Thank you! ['/ce-api?param=1', '', TRUE] actually failed. Fixed it and added a couple of test cases.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs review » Reviewed & tested by the community

Looks good.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Reviewed & tested by the community » Needs work

it's really great to have a test added here! I do think it's not correctly checking things though, plese take look.

utor.io’s picture

utor.io commented
Status: Needs work » Needs review

Commented and added tests for altered paths.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Needs work

I see, makes sense.

TEst coverage improvement is great, however not fully there:

e.g. from test-data

'Frontpage param CE-API trailing slash' => ['/ce-api/?param=1', '', TRUE, '/'],

We should see this resulting into /?param=1 - thus we want to test not only the resulting path but the resulting request-URI really. Could you adust the test-case like this? Then I think this is ready to go.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs work » Needs review

1 commit added (and force-pushed with PHPCS fix): added "?param=1" to all expected/tested values, where the input values had them. Nothing else.

I hope this is good to merge now, per the previous comment. It simplifies other issues that need to add phpunit tests.

  • fago committed c5e1dd21 on 1.x authored by arthur_lorenz 
    Issue #3409409 by arthur_lorenz, roderik, fago: Respect installations...
fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Fixed

ok, thanks. that works well, merged then.

Status: Fixed » Closed (fixed)

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