Since ECA allows modules to trigger events when a recipe is applied, there's a possibility that a module could call an internal route during installation. If that request boots up Drupal before the installation is complete, it could cause failures.
By ensuring external requests that bootstrap Drupal don’t interfere with the installation process, you're aiming to prevent those failures. That aligns well with the goal of issue [3502018].
@smustgrave I'm not sure if that's what you meant by "test case," but I attached two video recordings to this ticket demonstrating the issue.
Fix target branch to 11.x and test coverage implementation.
I believe I’ve identified the main issue here.
The InstallerKernel::installationAttempted()
function relies on the $GLOBAL
variable to check if the installer is running. However, this only works within the same script execution. When Cron or Drush commands are executed, those requests don’t have access to the $GLOBAL
variables set by the installer.
To resolve this, I propose updating InstallerKernel::installationAttempted()
to also use Config Storage. During installation, a temporary config would be created and removed upon completion. This approach would allow us to verify the installation status across multiple requests.
@catch It’s possible.
@pjmuszynski Running the cron via the Drush CLI will consistently cause the installation to fail. While this doesn’t seem like a valid scenario—since it's unlikely someone would execute Drush commands during the Drupal installation—any Drush command (e.g., drush st
) will also trigger a failure. This happens because Drush bootstraps Drupal to execute commands, which might align with what @catch mentioned earlier.
I created this core issue
🐛
Installation fails if cron runs in parallel
Active
.
Hope it can prevent this issue from happening.
hfernandes → created an issue.
Initial MR created with the proposed fix.
hfernandes → created an issue.
@catch, the cron was triggered during one of the steps of the Drupal CMS installation process. I’m not entirely sure what action caused this call to be made in the middle of the process—perhaps something like a window resize that lazy-loaded an asset?
It’s worth noting that the cron was triggered by a JS file load. Starting from Drupal 10.1, JS and CSS file loads are handled by two specific routes—system.css_asset
and system.js_asset
(reference:
https://www.drupal.org/node/3358091 →
). Should we consider preventing the automated_cron from being triggered if the route is one of these two?
in 11.2 we removed the hard-coded cron run, but it's executed on the first 'real' request after install (with core's installer, this is the last page of the installer) via automated cron
Could you share the issue number for that change? Additionally, are we considering that the automated_cron
module isn’t included in some installation profiles, such as minimal
?
Also, if I understand correctly, the issue in
📌
Search must be indexed after recipe is applied
Active
would only occur if you have a running website and then install the search recipe, as the install_finished
function triggers the cron job at the end of the installation process.
I'm not very familiar with ECA module, but can it take a state variable into account?
If it can, you might consider adding an additional validation step that checks whether the install_time
state variable has been set. This would ensure the cron job does not run during the installation process.
hfernandes → created an issue.
I’ve merged the 8.x-1.x branch back into our MR.
I also fixed a PHPCS issue, but there are still other reported issues in the ViewsDataExportCommands.php
file:
Namespaced classes/interfaces/traits should be referenced with use statements.
I left this issue unresolved, as it seems we don’t yet have a definitive decision on whether this is a concern. For reference, see 🐛 False Postive: Aliased PHP 8.0 Attributes on Class properties leading to "Namespaced classes/interfaces/traits should be referenced with use statements" Needs work .
Hi @jrockowitz,
Thank you for your suggestion regarding the index on name,property
. However, I believe that a composite index including both webform_id
and name
would provide better performance for the query in question. Here’s my reasoning:
Query Condition Analysis:
Based on the query you referenced, the WHERE
clause includes both webform_id
and name
. This means that both columns are involved in filtering the results.
While an index on name,property
may help in some scenarios, it does not fully cover the webform_id
filter condition.
A composite index on webform_id
and name
allows the database engine to use both columns together for filtering. This reduces the need for additional scans and increases the speed of the query, especially in large datasets where webform_id
is often a primary filter.
Without this composite index, the database would either:
- Perform a less efficient scan using a partial index (e.g., on
name
orproperty
alone). - Fall back to a full table scan if no suitable index exists.
Performance Testing Evidence:
To demonstrate the impact, I tested the query on a sample dataset. Below are the results:
Query without a composite index:
Query with a composite index on name
and property
:
Query with a composite index on webform_id
and name
:
These tests show a clear improvement in query performance when using the composite index on webform_id
and name
and no real impact - the performance got worse - when using a composite index on name
and property
.
Database Optimization Best Practices:
It’s a best practice to align indexes with the columns used in the WHERE
clause to minimize row scans.
Adding the composite index on webform_id
and name
would allow the query planner to leverage it effectively and deliver faster results.
I also agree that the use of the IN
clause in this case is unlikely to cause a significant impact on query performance. However, I’d like to point out that the post you referenced pertains to SQL Server, which uses a different query engine than MySQL. While some general indexing principles may overlap, it’s important to account for differences in how these databases optimize queries and use indexes. In MySQL, aligning indexes with the columns in the WHERE
clause is a crucial factor in achieving optimal performance on large datasets.
I am adding another related issue as the solutions are the same.
I'm updating the MR to include the is_string
validation since the function mb_strtolower
expects a string as parameter.
hfernandes → made their first commit to this issue’s fork.
@jrockowitz, would it make sense to merge both developments into a single MR? The ✨ [Performance] Add an index on webform_submission_data name and property Needs review issue introduces an index on the 'name' and 'property' values, while this MR adds an index on 'name' and 'webform_id.'.
hfernandes → created an issue.
Attaching the patch and opening the MR.
hfernandes → created an issue.
I've submitted the PR to Docksal as well: https://github.com/docksal/service-nginx/issues/22
MR opened.
hfernandes → created an issue.
I noticed that with the `if`, the `loop.index` wasn't counting correctly, so I changed it to use the `filter` filter instead.
Here is one example, the first accordion item should be open by default. With the `if` approach, it wasn't.
Using `filter`:
Moving it back to `Needs Review`.
hfernandes → made their first commit to this issue’s fork.
I've just installed this module and faced this issue.
Applying this patch I can confirm this issue has been fixed.
My setup:
- Drupal: 9.5.2
- Module version: 8.x-3.8
Adding the patch file.
MR created.
hfernandes → created an issue.
I did some tests and it's working as designed.
Note: if you have "Force image usage only" unchecked and disable the browser's Javascript, it loads properly (see attached).
MR created and patch tested.
Working as expected.
hfernandes → made their first commit to this issue’s fork.
I've fixed the MR https://git.drupalcode.org/project/drupal/-/merge_requests/2537
Also attaching the generated patch for Drupal 9.5 here.
hfernandes → made their first commit to this issue’s fork.