Hi @avpaderno,
Thank you for your clarification. I am closing this application as both maintainer and project owners are not qualified to apply.
I don't understand why that merge request matters as it was closed and it changes were not merged into the branch 1.0.x. Also, the majority of commits in that merge request were authored by me.
You could see many commits "Merge pull request..." from the project owner because the project was mainly developed on our GitHub repository. We have recently synced the code to Drupal Git when the project was ready as a public module.
If my commit count is not enough for this type of application, does the commit count of the project owner qualify her as an applicant?
Hi @avpaderno,
I'm not sure why my commit count is not considered to be enough to apply. The project has had 75 commits since inception and 32 were authored by me. Since September 2024 (6 months ago), the majority of the commits were solely authored by myself. About 3 months ago, most of the code was rewritten (from this commit a66c7e8b) and almost all PHP code was authored by me.
Are you able to check the commit history of the project to verify my point, please?
I am changing priority as per Issue priorities → .
Hi @vishal.kadam,
- The
mainis a legacy branch and not in use. The main development branch is1.xand the requested branch is1.0.x. I have, however, deleted themainbranch from Drupal git. - I have updated the code as per your suggestions in the latest commit https://git.drupalcode.org/project/webform_openfisca/-/commit/5aaada2867...
For the coding standard issues, are they enforced by any Drupal/DrupalPractice PHPCS rules? We would like to enable those checks in the current CI so that they can be caught during pipeline runs.
Thank you for your review.
I've figured out that this issue also affects radio buttons.
Confirm the issue when using the JSON data_parser with item_selector: /. getResponseContent() returns the correct data but getSourceData() always returns an empty array.
My debugging also leads to the same line which was changed in this MR https://git.drupalcode.org/project/migrate_plus/-/merge_requests/81/diff....
As a workaround, I have to use item_selector: '' to make migrations work again.
- Remove the patch from #3 as it's incorrect
- Incorporate Issue #3114500 by sonnykt: Add a settings for indexed_chars to extract more than 100K characters.
- Incorporate Issue #3105993 by yovince, sonnykt, ciprian.stavovei: Fatal error when working with indices with custom cluster prefix/suffix.
- Incorporate Issue #3100797 by sonnykt, nadim hossain: Allow other modules to alter the list of attachment of the node to be indexed..
sonnykt → made their first commit to this issue’s fork.
Contextual is not a crucial module however it's one of the best tools to assist site builders. It's the easiest way for site builders to manages elements on a page regardless how the page is built/assembled. Without proper contextual link support, it's a hassle to figure out where to manage a piece of information on a page. Is it a block, a view, an entity, or just some static content? With Layout Builder, a page can be assembled from all kinds of building element and Contextual links help site builders not scratching their head digging in the admin area to figure out where to manage a small piece of content.
This looks like a defect. 1.8 has brought Styleguide and testing of form elements, including testing of passing of attributes.
The attributes passing itself was refactored to handle more cases. Maybe this is not an issue anymore.
The snippets were produced from CivicTheme 1.8.
I agree in principle. However, "the site owner" is not always a developer - sometimes it is a script within an automation that just launches the site, sometimes it is a human evaluator without any technical skills. ...
In that case, the site owner is also the person decided to use CivicTheme in the first place. When facing a technical issue e.g. whether to follow CivicTheme rules for an unsupported contrib module, a developer is always involved.
If there was no stripping-out of classes - the functionality will not work (there is no component in CivicTheme, for example), a developer would dive into code and realise that some work needs to be done to add a support for this (new component etc.), but the site could be affected because a site builder could have accidentally misconfigured the contrib leading to "bleeding" styles.
That is not true. If CivicTheme didn't strip out classes, unsupported contrib modules would always work. Their output would be just unstyled. It's the expected behaviour of all Drupal themes. Developers only need to look into the theme hook of the said module to figure out how to style the theming elements.
In reality, if a module defines its own theme element in hook_theme, it is likely broken by CivicTheme due to the stripping out of classes behaviour. However, developers won't know why the expected functionality doesn't work and they have to dig into the module code. Only debugging could reveal the ultimate culprit which is civictheme_preprocess_last().
CivicTheme implements about 50 template_preprocess_ hooks. Instead of stripping out the classes/attributes from the 50 supported theme elements (aka opt-in fashion), CivicTheme goes with the ultimate but lazy approach to remove everything with template_preprocess_last(). It doesn't care whether a theme element is supported by CivicTheme. Just remove them all.
There are thousands of contrib module out there defining their own theme elements with hook_theme(). By design, CivicTheme will likely break them all if their users (read: developers) don't write a template_preprocess_ hook to opt-out for every theme element. When a Drupal theme by design tends to break "unsupported" contrib modules, I would seriously lower the confidence factor when assessing an unsupported contrib module for a site with CivicTheme.
The CSS classes though - they should not be used for anything else but presentation. And CivicTheme provides that presentation using own classes. If a contrib uses CSS to bind JS events or any other custom functionality - they are doing it wrong by occupying the space used purely for presentation. And they should fix it to use attributes rather than classes. The `js-*` is an old-school approach from times when custom attributes were not supported by browsers. It should not be used in the modern development practices.
It doesn't matter if it's an "old-school" approach. There are thousands of contrib modules out there still relying on the "js-*" classes. Drupal core is still using "js-*" classes. It's not the job of a design system to tell core and contrib modules how to implement their functionalities. Why can other Drupal themes of other design systems work with most contrib modules but not CivicTheme?
@sime afaik CivicTheme has limited support for contextual links on Layout Builder only.
The issue is still occurring with MySQL 8:
MySQL [v8032]> SELECT VERSION();
+-----------+
| VERSION() |
+-----------+
| 8.0.32 |
+-----------+
1 row in set (0.000 sec)
MySQL [v8032]> SELECT @@character_set_database, @@collation_database;
+--------------------------+----------------------+
| @@character_set_database | @@collation_database |
+--------------------------+----------------------+
| utf8mb4 | utf8mb4_0900_ai_ci |
+--------------------------+----------------------+
1 row in set (0.001 sec)
MySQL [v8032]> SHOW CREATE TABLE redirect_404\G
*************************** 1. row ***************************
Table: redirect_404
Create Table: CREATE TABLE `redirect_404` (
`path` varchar(191) NOT NULL COMMENT 'The path of the request.',
`langcode` varchar(12) CHARACTER SET ascii COLLATE ascii_general_ci NOT NULL DEFAULT 'und' COMMENT 'The language of this request.',
`count` int unsigned NOT NULL DEFAULT '0' COMMENT 'The number of requests with that path and language.',
`timestamp` int unsigned NOT NULL DEFAULT '0' COMMENT 'The timestamp of the last request with that path and language.',
`resolved` int NOT NULL DEFAULT '0' COMMENT 'Boolean indicating whether or not this path has a redirect assigned.',
`daily_count` int unsigned NOT NULL DEFAULT '0' COMMENT 'The number of requests with that path and language in a day.',
PRIMARY KEY (`path`,`langcode`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci COMMENT='Stores 404 requests.'
1 row in set (0.001 sec)
#*** one trailing space 0x20
MySQL [v8032]> INSERT INTO redirect_404 VALUES ('/test-trailing ','en',0,0,0,0);
Query OK, 1 row affected (0.004 sec)
#** two trailing spaces 0x20
MySQL [v8032]> INSERT INTO redirect_404 VALUES ('/test-trailing ','en',0,0,0,0);
Query OK, 1 row affected (0.002 sec)
#** one trailing nbsp 0xC2A0
MySQL [v8032]> INSERT INTO redirect_404 VALUES ('/test-trailing ','en',0,0,0,0);
ERROR 1062 (23000): Duplicate entry '/test-trailing -en' for key 'redirect_404.PRIMARY'
MySQL [v8032]> SELECT "a " = "a ";
+--------------+
| "a " = "a " |
+--------------+
| 1 |
+--------------+
1 row in set (0.001 sec)
MySQL [v8032]> SELECT "a " = "a ";
+--------------+
| "a " = "a " |
+--------------+
| 0 |
+--------------+
1 row in set (0.001 sec)
The special character is 0xC2A0 (UTF-8 non-breaking space).
The issue occurs with Panelizer. I understand that Panelizer is no longer in active development however there are many websites out there have not moved away from that module yet. They didn't have this issue with Metatag 1.x but after upgrading to Metatag 2,0 the issue reappears.
@alex.skrypnyk It doesn't matter how the unsupported elements are rendered in default CivicTheme. It's up to the site owner to decide whether to repurpose an existing CivicTheme component, extend a component, develop a new component, or even ignore the UIKit rules and go with a custom styling. Using an "unsupported" contrib module with CivicTheme is a conscious choice and no reasonable person will blame CivicTheme for not displaying the unsupported elements correctly.
It's totally fine for the elements to not display correctly with default CivicTheme. The minimum expectation is to have an unstyled but functional element. Developers will need to style the said elements for sure but they need the functionality to work before it reaches the styling stage. However, it's unacceptable for a backend functionality to be broken because a theme strips the necessary information from the output. Backend code doesn't set the class attribute for nothing, especially when it sets js-* classes. It's even worse when the theme has no support of the said elements but it still change the element anyway. Developers don't know why the backend functionality stops working until they spend time on debugging the contrib module and reckon that the culprit is not a module bug but from a theme. The idea of developers now have to look into theme code to investigate a backend issue doesn't sound good at all.
CivicTheme doesn't just strip the class attribute. It also strips other "unsupported" attributes. This behaviour is causing issues with both core and contrib modules. Facets module can't transform its facet links into checkboxes because of a missing js- class. Views exposed text filter loses its placeholder attribute because StringFilter uses ['#attributes']['placeholder'] instead of #placeholder. Better Exposed Filter module's checkboxes filter loses their checked state with CivicTheme. I'd assume that any contrib module that relies on the attributes will be likely impacted (if not broken) with default CivicTheme.
The worst impact is standard Form API losing all meaningful and valid attributes that have nothing to do with styling:
$form['log'] = [
'#type' => 'textfield',
'#title' => $this->t('Log message'),
'#size' => 30,
'#attributes' => [
'my-custom-attributes' => 'abc',
'data-abc' => 1,
'pattern' => '[A-Za-z]{3}',
'minlength' => 10,
'spellcheck' => 'true',
'placeholder' => 'Enter a log message',
]
];
The above element is expected to have the following output with core and regular contrib themes:
<!-- BEGIN OUTPUT from 'core/modules/system/templates/input.html.twig' -->
<input my-custom-attributes="abc" data-abc="1" pattern="[A-Za-z]{3}" minlength="10" spellcheck="true" placeholder="Enter a log message" data-drupal-selector="edit-log" type="text" id="edit-log" name="log" value="" size="30" maxlength="128" class="form-text form-element form-element--type-text form-element--api-textfield" tabindex="-1">
With CivicTheme, the output is minimal and meaningless:
<!-- BEGIN OUTPUT from 'themes/contrib/civictheme/templates/form/form-element--civictheme-field.html.twig' -->
<input type="text" class="ct-textfield ct-theme-light ct-field__control form-text" name="log" id="edit-log" size="30" maxlength="128" data-abc="1" data-drupal-selector="edit-log" tabindex="-1">
I would argue that the default behaviour should be "opt-in" instead of "opt-out". CivicTheme can remove the class attribute from all the elements in the preprocess hooks it implements but it should not touch the elements it has no support.
For example, CivicTheme is breaking the Facets module. When using with CivicTheme, Facets module cannot transform its facet links into checkboxes because the required class attribute is removed by CivicTheme. Facets' theme hook facets_item_list and its template facets-item-list.html.twig are not supported by CivicTheme. CivicTheme is not aware of the hook/template and does not implement the _preprocess_facets_item_list hook either. However, it still removes the class attribute from the element of the theme hook and breaks the default functionality.
As a workaround, a developer has to implement a _preprocess_facets_item_list hook with $variables['modifier_class'] = FALSE; to opt-out although Facets does not interfere with CivicTheme and CivicTheme does not support Facets.
@FTE
Confirming that turning sql_generate_invisible_primary_key off on Azure Flexible server fixed the issue on our site.
@mjpastor You may want to check the structure of the cache_page table especially the cid column:
$ drush sqlc
> show create table cache_page;
The cid column should be like this (on MariaDB):
`cid` varchar(255) CHARACTER SET ascii COLLATE ascii_bin NOT NULL DEFAULT '' COMMENT 'Primary Key: Unique cache ID.',
If your cid column looks like the below, you can drop your cache_* tables then drush cr will correctly recreate them and the issue will be gone:
`cid` varchar(255) NOT NULL DEFAULT '' COMMENT 'Primary Key: Unique cache ID.',
Using MR88 as a composer patch fixes the issue on our Drupal 10.2.4 site.
"drupal/redirect": {
"Broken redirect from RouteNormalizerRequestSubscriber when ajax_page_state present and $request->overrideGlobals() has been called - https://www.drupal.org/project/redirect/issues/3426938#comment-15483275": "https://git.drupalcode.org/project/redirect/-/merge_requests/88.diff"
},
alex.skrypnyk → credited sonnykt → .
Merge request 6875 is for Drupal 10.2.x.
Merge request 6876 is for Drupal 11.x.
@claudiu.cristea For #7, it was an issue specifically on our system as we used a script (deploy hook) to generate document versions. The hook in #7 was tailored for our data so I couldn't provide a patch for entity_legal as I wasn't sure about the data on other systems. I think it's acceptable for site maintainers to check the data and apply the hook at their discretion.
I've opened an MR to fix another deprecation notice in the submodule matomo_tagmanager.
It's impossible to rebase the old MR https://git.drupalcode.org/project/drupal/-/merge_requests/3109 onto D11 hence I cherry-picked the top commit into a new MR against 11.x branch.
It seems that Drush detection in the issue #3036504 → no longer works with both Drush 11 and 12.
// When indexing via Drush, multiple iterations of a batch will happen in
// the same PHP process, so the static cache will quickly fill up. To
// prevent this, clear it after each batch of items gets indexed.
if (function_exists('drush_backend_batch_process') && batch_get()) {
\Drupal::getContainer()->get('entity.memory_cache')->deleteAll();
}
When running cron with Drush 11/12, the memory usage keeps increasing and OOM is inevitable:
[info] Starting execution of search_api_cron(), execution of scheduled_transitions_cron() took 1.67ms. [5.65 sec, 48.97 MB]
[info] Indexed 20 items for index Classification Title. [5.93 sec, 54.54 MB]
[info] Indexed 20 items for index Classification Title. [6.04 sec, 56.88 MB]
...
[info] Indexed 20 items for index Classification Title. [12.76 sec, 218.94 MB]
[info] Indexed 20 items for index Classification Title. [12.9 sec, 221.23 MB]
[info] Indexed 20 items for index Classification Title. [12.97 sec, 223.53 MB]
[info] Indexed 20 items for index Classification Title. [13.04 sec, 225.84 MB]
...
[info] Indexed 20 items for index Classification Title. [19.74 sec, 354.88 MB]
[info] Indexed 20 items for index Classification Title. [19.94 sec, 357.15 MB]
[info] Indexed 20 items for index Classification Title. [20.01 sec, 359.45 MB]
PHP Fatal error: Allowed memory size of 419430400 bytes exhausted (tried to allocate 20480 bytes) in /app/web/core/lib/Drupal/Core/Cache/CacheTagsChecksumTrait.php on line 131
PHP Fatal error: Allowed memory size of 419430400 bytes exhausted (tried to allocate 32768 bytes) in /app/web/modules/contrib/shield/src/ShieldMiddleware.php on line 1
Applying the patch from #4 🐛 OOM issue after creating approx 5000 nodes Needs review seems to fix the OOM issue.
[info] Starting execution of search_api_cron(), execution of scheduled_transitions_cron() took 1.52ms. [4.86 sec, 49.08 MB]
[info] Indexed 20 items for index Classification Title. [5.03 sec, 54.39 MB]
[info] Indexed 20 items for index Classification Title. [5.12 sec, 54.92 MB]
[info] Indexed 20 items for index Classification Title. [5.2 sec, 54.96 MB]
...
[info] Indexed 20 items for index Classification Title. [9.8 sec, 56.54 MB]
[info] Indexed 20 items for index Classification Title. [9.88 sec, 59.44 MB]
[info] Indexed 20 items for index Classification Title. [9.96 sec, 56.48 MB]
...
[info] Indexed 20 items for index Classification Title. [18.84 sec, 57.4 MB]
[info] Indexed 20 items for index Classification Title. [18.92 sec, 60.31 MB]
[info] Indexed 20 items for index Classification Title. [19.01 sec, 57.22 MB]
[info] Starting execution of search_api_solr_cron(), execution of search_api_cron() took 14144.63ms. [19.01 sec, 57.22 MB]
[info] Starting execution of simple_sitemap_cron(), execution of search_api_solr_cron() took 15.27ms. [19.03 sec, 57.9 MB]
sonnykt → created an issue.
Patch #20 cannot be applied to Drupal 10.1.5.
Patch #16 can be applied and fixes the issue.
Re-rolled patch file from #116 and #133 on the latest 8.x-2.13. This new patch now uses the root base_url setting from entity_print.
I don't have this error with 4.0-alpha1 but face a similar issue with the same update hook:
> [notice] Update started: entity_legal_update_9002
> [error] SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1677569101' for key 'PRIMARY': UPDATE "entity_legal_document_version" SET vid = CAST(SUBSTRING_INDEX(name, '_', -1) AS UNSIGNED); Array
Turned out that we have several document versions with the same timestamp prefix because they were created programmatically at the same time by a script. We have to add a custom update hook running before 9002 to solve the issue.
function osp_core_update_dependencies() : array {
$dependencies = [];
$dependencies['entity_legal'][9002] = ['mymodule' => 9001];
return $dependencies;
}
function mymodule_update_9001() : void {
// The update hook 9002 from entity_legal adds an incremental ID column to
// the entity_legal_document_version table. It casts the last substring after
// the _ in the name column into int. That substring was generated using the
// current timestamp when the document version was create. However, there are
// several entries with same timestamp as they were generated at the same time.
// We have to change that timestamp so that entity_legal_update_9002() does
// not have the integrity constraint violation error.
// @see entity_legal_update_9002().
$versions = [];
$database = \Drupal::database();
$query = $database->select('entity_legal_document_version', 'v');
$query->addExpression("SUBSTRING_INDEX(name, '_', -1)", 'timestamp');
$query->addExpression("GROUP_CONCAT(name)", 'names');
$query->groupBy('timestamp')
->having('COUNT(timestamp) > 1');
$results = $query->execute()->fetchAll();
foreach ($results as $result) {
$timestamp = (int) $result->timestamp;
$names = explode(',', $result->names);
foreach ($names as $delta => $name) {
$new_name = strrev($name);
$new_name = preg_replace(strrev('/_' . $timestamp . '/'), strrev('_' . ($timestamp + $delta)), $new_name, 1);
$new_name = strrev($new_name);
$versions[$name] = $new_name;
}
}
if (empty($versions)) {
return;
}
// Gather the tables to update the document names.
$tables = [
'entity_legal_document_version' => ['name'],
'entity_legal_document_version_data' => ['name'],
'entity_legal_document_acceptance' => ['document_version_name'],
];
// Additional tables for fields.
$key_value = \Drupal::keyValue('entity.storage_schema.sql');
$entity_type = 'entity_legal_document_version';
$field_name = 'entity_legal_document_text';
$storage_schema = $key_value->get($entity_type . '.field_schema_data.' . $field_name);
foreach ($storage_schema as $table => $schema_data) {
if ($database->schema()->tableExists($table)) {
$tables[$table] = ['entity_id', 'revision_id'];
}
}
// Update document_version name to new name.
foreach ($tables as $table => $columns) {
foreach ($versions as $name => $new_name) {
$query = $database->update($table);
$fields = [];
foreach ($columns as $column) {
$fields[$column] = $new_name;
$query->condition($column, $name);
}
$query->fields($fields);
$query->execute();
}
}
}
The patch from the MR works in terms of appending the destination query param to the one-time login link. However, it breaks the login process with passwordless.
Steps to reproduce:
* Visit a page not available for anonymous users eg. /node/123
* Passwordless will redirects the user to /user/login?destination=/node/123
* Fill an email to the passwordless login form
* The email generated by passwordless has the expected one-time login URL: /user/reset/<UID>/<TIMESTAMP>/<HASH>?destination=/node/123
* Visit the above one-time login link
Expected result
* User is logged in and on the page /node/123
Actual result
* The user remains anonymous and is sent to /node/123 then redirected to /user/login?destination=/node/123 with the passwordless login form
Cause of the issue
* When the anon user visits the one-time login link, they are on the user.reset route
* Passwordless then redirects the anon user to the user.reset.login route with a RedirectResponse (in PasswordlessUserController::resetPass() method).
* As the RedirectResponse has the desired destination query param, it is modified by the RedirectResponseSubscriber::checkRedirectUrl() event subscriber. That subscriber lets the 'destination' query parameter override the redirect target. Hence, the anon user is redirected to the 'destination' query instead of the user.reset.login route.
I think we only need to remove the boosting from MappingFactory. Removing/disabling the boosting config from the index form will impact the query-time boosting in SearchBuilder::getSearchQueryOptions().
The patch from
#5
📌
Index time boost is deprecated
Needs review
also removes query-time boosting from SearchBuilder which I don't think it's correct.
gargsuchi → credited sonnykt → .
@Halor I noticed that ckeditor_templates config entities capture the File ID of the image for thumb. I don't think that storing the incremental ID in config is a good practice. It's not an easy task to keep the FID in sync between different environments.
Previously, the CKEditor Template UI module stores the path of the image. Other module such as paragraph stores the UUID of the icon and/or the base64-encoded version of the image.
Not sure if I do anything wrong but I can only apply patch #40 and #41 onto version 1.2.0. None of other patches including the MR can be applied with composer.
Patch #24 cannot apply to Footnotes 3.0.1.
Migrations can be rolled back via UI and I'm afraid that Content Lock still breaks migrations.
IMO, a low-level CRUD hook is not an appropriate solution as it can break many operations using the entity_delete(), eg. bulk operations, cron, queue workers, etc.
Kristen Pol → credited sonnykt → .
I notice that Entity Print 8.x-2.11 introduced a new root setting Base URL:
It also has the entity_print.post_render_subscriber event subscriber to change all root relative URIs in <link href>, <a href>, and <img src> into absolute URLs using the new base URL setting above.
I have tried to remove the patch from this issue and instead use that new Base URL setting and notice that dompdf is still able to load CSS/images on my Docker-based project.
I wonder if this issue is still valid with the new setting.
@Rar9 probably check for malformed HTML. The error message from dompdf likely relates to the HTML structure instead of remote assets.
@Rar9 @kwinten-hardies you should probably check the output HTML for printing. Do you have HTML5 Parser enabled for Dompdf plugin?