Account created on 11 September 2012, over 12 years ago
  • Software Architect at ImageX 
#

Merge Requests

More

Recent comments

🇧🇷Brazil hfernandes

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].

🇧🇷Brazil hfernandes

@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.

🇧🇷Brazil hfernandes

Fix target branch to 11.x and test coverage implementation.

🇧🇷Brazil hfernandes

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.

🇧🇷Brazil hfernandes

@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.

🇧🇷Brazil hfernandes

I created this core issue 🐛 Installation fails if cron runs in parallel Active .
Hope it can prevent this issue from happening.

🇧🇷Brazil hfernandes

@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?

🇧🇷Brazil hfernandes

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.

🇧🇷Brazil hfernandes

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.

🇧🇷Brazil hfernandes

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 .

🇧🇷Brazil hfernandes

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 or property 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.

🇧🇷Brazil hfernandes

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.

🇧🇷Brazil hfernandes

@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.'.

🇧🇷Brazil hfernandes

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`.

🇧🇷Brazil hfernandes

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

🇧🇷Brazil hfernandes

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).

🇧🇷Brazil hfernandes

MR created and patch tested.
Working as expected.

Production build 0.71.5 2024