- Issue created by @tstoeckler
- 🇩🇪Germany tstoeckler Essen, Germany
Not opening an MR yet before getting some confirmation on the direction here (and also because the workaround is so trivial that I don't need a patch 😉).
If you use a customized the JSON:API path prefix as part of (exported) site configuration and then install an instance of that site with Drush using the --existing-config option, the routes after installation will have the default path prefix (/jsonapi) instead of the customized one. Clearing the cache explicitly subsequently to installation rectifies the issue.
drush si --existing-configExpected result:
The API output
Actual result:
Page not found
\Drupal\jsonapi_extras\EventSubscriber\ConfigSubscriber::onSave(), which otherwise rebuilds the router when the path prefix changes, explicitly bails out during the installation, i.e. if \Drupal::getContainer()->getParameter('kernel.environment') === 'install'. Simply removing that check resolves the issue.
The check was introduced in #2982133: No longer works with JSON API >=1.21 (so either 1.21 or 1.22) → and more specifically commit d746ffc0. The only rationale → for that commit is:
This broke Contenta CMS installer using CLI.
(referring to a previous commit from the same issue). Notably, that comment is 6 and a half years old at the time of writing.
So as far as I can tell there is a good chance that the problem that this check is trying to avoid no longer exists. In fact the --existing-config option did not exist in Drush at the time of that comment (in a strange accident of fate it was actually released mere three (!) days later) in which case it could just be removed.
As a related (albeit somewhat distinct) issue, I also verified that instead of calling RouteBuilder::rebuild() it is sufficient to call setRebuildNeeded() for the routes to end up being built correctly, so if there is some reservation against doing the route building during command-line installation that may help to alleviate that concern.
-
-
-
Active
3.0
Code
Not opening an MR yet before getting some confirmation on the direction here (and also because the workaround is so trivial that I don't need a patch 😉).