Unnecessary asserts break installation in distros that include JSON API

This is related to #2986484: Add verbose logging to failed assertions for base path and https://github.com/contentacms/contenta_jsonapi/issues/301, which triggered both of these.

This was done to unblock https://github.com/contentacms/contentajs — see http://humanbits.es/web-development/2018/07/16/contentajs/.

The asserts fail during installation of a site that includes JSON API due to the container not being fully ready. That makes the container parameter to be checked before it's actually necessary.

Then rather than removing them, let's assert them only if the container is fully ready if at all possible.

Before #2986484, the error looked like this:

Warning: assert(): assert(substr($jsonapi_base_path, -1) !== '/') failed in Drupal\jsonapi\Routing\Routes->__construct() (line 83 of modules/contrib/jsonapi/src/Routing/Routes.php).

What did it look like afterwards? That would tell us what value the assert() was receiving. We need that information to determine whether we want this to have a follow-up, and if so, what that follow-up should do.

I'm happy to do all that, but I can't without the necessary information :) Hence marked Postponed (maintainer needs more info). Could you post that error output? Thanks!

Thanks for posting that info — I'll follow up on this on Sunday or Monday :) Marking Needs work to reflect that I'm about to dig into it to understand how it could be set to that value.

I bet that was because \Drupal\jsonapi_extras\JsonapiExtrasServiceProvider::alter() does:

    $settings = BootstrapConfigStorageFactory::get()
      ->read('jsonapi_extras.settings');
    $container->setParameter('jsonapi.base_path', '/' . $settings['path_prefix']);

It's conceivable that very early during the installation process, $settings is still empty.

In fact, I can reproduce this even outside the installation process of a distro: with just vanilla Drupal core + jsonapi, then installing jsonapi_extras results in this:

This is therefore a bug in jsonapi_extras, not jsonapi. It was introduced in #2982133: No longer works with JSON API >=1.21 (so either 1.21 or 1.22), which was done in response to #2971745: Don't hardcode `/jsonapi/` in FormatSetter, read JSON API base path from container parameter instead.

D'oh.

Forgot the screenshot for #9.

So, ideally:

• We'd revert the #3 + #5 JSON API commits.
• We'd commit #9 to JSON API Extras.

(Sorry for the late reply, I was on vacation since July 24 until this past weekend!)

Why would $jsonapi_base_path be set to NULL? jsonapi.services.yml sets it to "/jsonapi". That's guaranteed to be the value, unless a ServiceModifierInterface implementation alters it. And that's what jsonapi_extras was doing! And the only reason it even ever caused a problem, is because it overrides that container parameter based on configuration, which indeed is kind of tricky to access during installation. Hence the patch in #9 fixes this.

(I tested the patch in #9 with jsonapi_extras — before that patch core + jsonapi + jsonapi_extras would indeed cause the problem you originally reported; with that patch it's fixed! 🙂)

Per #9, created #2991767: jsonapi.base_path container parameter should only be overridden if JSON API Extras' configuration is available to fix the bug in JSON API Extras.

The next step here is to revert this patch. I'm fine with leaving this patch committed to 1.x even though it never should've been committed, nor should https://www.drupal.org/project/jsonapi/releases/8.x-1.23 have been released.
But in the 2.x branch, I want these assertions restored, to prevent future confusion and painful debugging. That's the reason those assertions exist!

So,:

1. committing #2991767: jsonapi.base_path container parameter should only be overridden if JSON API Extras' configuration is available to JSON API Extras, and tagging a release: this fixes the root cause
2. committing the attached revert patch to the 2.x branch ensures that we have these assurances again for future JSON API versions
3. optionally we could also commit a revert patch to the 1.x branch

Confirmed that the code path in JSON API Extras was fixed to resolve this issue.

LGTM

Thanks to @e0ipso for committing #2991767: jsonapi.base_path container parameter should only be overridden if JSON API Extras' configuration is available!

The patch needs a reroll by now. #2987205: FormatSetter doesn't set the format to `api_json` when accessing just `/jsonapi` removed the FormatSetter class. Fortunately, that means we can just move the more verbose (and more helpful!) assertions from FormatSetter to Routes. IOW: the patch just becomes smaller and achieves the same.

4 failures due to something that is freshly deprecated in 8.6 and 8.7 as of today: #2998888: Drupal\field\Tests\EntityReference\EntityReferenceTestTrait deprecated. Asked for advice how to deal with it in #2996789: Deprecate Drupal\field\Tests\EntityReference\EntityReferenceTestTrait.

Tests are actually passing. Failures are just for deprecation notices.

Working to get those 4 failures due to #2996789: Deprecate Drupal\field\Tests\EntityReference\EntityReferenceTestTrait resolved in #3001958: 4 test fails due to using deprecated code on 8.6 and 8.7 since #2996789: temporarily fork the test trait.

#3001958: 4 test fails due to using deprecated code on 8.6 and 8.7 since #2996789: temporarily fork the test trait landed, retesting.

🎉