Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bootstrap Drupal correctly from inside SimpleSAMLphp #98

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

longwave
Copy link

@longwave longwave commented Apr 15, 2024

This was previously reported in #75 but closed as outdated, however it is still an issue.

When certain caches are cold or stale, it is possible for them to be rebuilt during a request that is wrapped by SimpleSAMLphp instead of Drupal's own index.php. However when this happens, the cache is not rebuilt correctly and are missing critical data. This then results in various errors related to missing fields on the Drupal site such as:

Attempted to create an instance of field with name field_XXX on entity type profile when the field storage does not exist.
Attempted to create an instance of field with name body on entity type block_content when the field storage does not exist.
Field layout_builder__layout is unknown.

or errors with missing plugins:

The "field_item:language" plugin does not exist. Valid plugin IDs for Drupal\Core\TypedData\TypedDataManager are: ...

This is because DrupalHelper::bootDrupal() does not fully bootstrap Drupal correctly before calling core services which may go on to initialise caches. In particular the DrupalKernel::preHandle() method is not called which is responsible for loading all modules, which causes the rebuilt caches to skip items from those modules and cause the errors later down the line.

As per the comment if you look at FunctionalTestSetupTrait::initKernel() or even /authorize.php you will see the correct way of partially bootstrapping Drupal without calling DrupalKernel::handle() is to call preHandle() (which calls loadLegacyIncludes() but also does a bit more).

Copy link

sonarcloud bot commented Apr 15, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@RoSk0
Copy link
Collaborator

RoSk0 commented Apr 16, 2024

Hi Dave,

Thanks for this!

I have a client with a similar symptoms where we suspected Memcached to be an issue, but I haven't had a chance to look at the problem in detail.

I will try to test this sometime soon.

Do you have a steps to reproduce this? As I'm missing reliable step on the mentioned project.

@longwave
Copy link
Author

longwave commented Apr 16, 2024

Yeah, I don't have reliable steps to reproduce, but anecdotally it did happen more frequently when the discovery cache bin was stored in Redis instead of the database. This had been happening for several months on a production site, but was only triggered maybe once or twice a month, and did not happen in other environments.

I tracked it down by swapping out EntityFieldManager and logging a stack trace in ::cacheSet() when an entity that I knew should have fields was cached with no fields. When this happened the stack trace always started in SimpleSAMLphp's module.php. This led me to #75 and since applying the MR here we haven't seen the issue again.

I have since tried to reproduce locally by manually clearing caches at specific times but so far have had no luck.

When discussing this in Drupal Slack I was previously pointed to https://www.drupal.org/project/drupal/issues/3207813 - this could also be considered a bug in Drupal as it should not try to build caches without checking all parts are fully loaded, but I think for the most robust fix we should bootstrap Drupal correctly from within this SimpleSAMLphp module as well.

@longwave
Copy link
Author

Further to this, this doesn't entirely solve the issue, there is a further bug in Drupal core: https://www.drupal.org/project/drupal/issues/3441833

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants