🇺🇸United States @scott_earnest

Account created on 13 January 2009, almost 16 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States scott_earnest

nvm sorry i totally missed this:

https://www.drupal.org/project/facets/issues/3455217 📌 Automated Drupal 11 compatibility fixes for facets Active

🇺🇸United States scott_earnest

Here is another method, that updates the aggregated filename every time the settings.deployment_identifier is updated.

Sorry if I put in the wrong issue both were marked fixed but thought it may help someone. Same patch applies to 10.3.x and 11.0.x:

https://www.drupal.org/project/drupal/issues/3371822#comment-15807906 📌 Add the deployment_identifier to CSS and JavaScript aggregate hashes Closed: duplicate
https://www.drupal.org/files/issues/2024-10-09/3371822-8-add-the-deploym...

🇺🇸United States scott_earnest

In our case, we wanted to use the deployment_identifier a little more aggressively, and ALWAYS use it (if set) on every asset. We were running into issues on the build where assets (css etc) were changed in a library whose version was NOT changed, causing the aggregated filename to not change - and would be cached on a CDN without the update to the style. We wanted all assets to be invalidated on every build, and controlled by the deployment_identifier which would change on each build.

The attached patch is another version of @catch 's patch (thank you) from #5. Instead of only using the deployment_identifer when the version is unset, this will use it on every asset. The fallback is the hash of the file contents (the base file has changed: AssetGroupSetHashTrait.php since other patch). Using a deployment_identifer should also decrease the expense of the build, since the system will never have to hash the contents of a file for the version.

PATCH TESTING

Setup
- Core aggregation (css an js) turned on
- Ensure caching is enabled
- Enable a module that has a "libraries.yml" file that includes an asset like CSS
- Set library version numeric e.g. "10.3.6", OR constant "VERSION"
- Look in the source for aggregated file(s), similar to:

    <link rel="stylesheet" media="all" href="/sites/default/files/css/css_hnAzTWJ78jrYDVrHTP6y05aGIS9LmOJ9kqQNNmNPny0.css?delta=0&amp;language=en&amp;theme=olivero&amp;include=eJxdjEEOAyEMAz-0aJ-EArg0aiBVgkD8vj1RqTd7RrZvH2h3IselwhOmdxVNJMHHFu714LcuGEpIOyTR_DriAZRTsraGPvxqKExxcoHGxtm0aSG5fzEKJyNj-Bl3mlxpsPbgyNoL2T7SQZafYX0f_1knM10f_05QDA" />
    <link rel="stylesheet" media="all" href="/sites/default/files/css/css_a1-EIU3hM01tk-bHNLV5AeTBWpvOtvaolQAlOLBNeD4.css?delta=1&amp;language=en&amp;theme=olivero&amp;include=eJxdjEEOAyEMAz-0aJ-EArg0aiBVgkD8vj1RqTd7RrZvH2h3IselwhOmdxVNJMHHFu714LcuGEpIOyTR_DriAZRTsraGPvxqKExxcoHGxtm0aSG5fzEKJyNj-Bl3mlxpsPbgyNoL2T7SQZafYX0f_1knM10f_05QDA" />

- Note these file names, and when they change during the test

Without Patch

- Update library asset e.g. CSS, clear cache
- Aggregated filename does not change

With Patch

- Update deployment_identifier, clear cache
- Aggregated filename changes

PATCH FILE:
- 3371822-8-add-the-deploymentidentifier.patch

🇺🇸United States scott_earnest

@effulgentsia - thanks for that suggestion! That's cool. I updated the MR with the anonymous function method.

Leaving in "Needs review" for now if someone else wants to test. If so make sure to follow the setup from the parent issue please and thank you:
https://www.drupal.org/project/mysql57/issues/3477448 🐛 Error when target is an array of connections Active

🇺🇸United States scott_earnest

Yes sorry - this was off the patch I created. I did not realize that this introduced a deprecation with a drush warning when clearing the cache.

I determined that a site's settings.php file get's loaded twice? Not sure why this was but because of this all settings get loaded twice.

I was not sure the best way to extract the "convert" piece in an .inc file - but this obviously causes problems with including it twice.

I though a path forward would be to check for "function exists", and also rename specific to the module.

🇺🇸United States scott_earnest

Attaching test results

- Drupal version: 11.0.4
- PHP version: 8.3.11

Prior to including the mysql57 settings.inc file, you can see that the "replica" database is an array of connections:

Defined in the settings.php file as such- per guidance from d.o:
- https://www.drupal.org/docs/drupal-apis/database-api/database-configuration

$databases['default']['default'] = [
  'driver' => 'mysql',
  'database' => 'drupaldb1',
  'username' => 'username',
  'password' => 'secret',
  'host' => 'dbserver1',
];
$databases['default']['replica'][] = [
  'driver' => 'mysql',
  'database' => 'drupaldb2',
  'username' => 'username',
  'password' => 'secret',
  'host' => 'dbserver2',
];
$databases['default']['replica'][] = [
  'driver' => 'mysql',
  'database' => 'drupaldb3',
  'username' => 'username',
  'password' => 'secret',
  'host' => 'dbserver3',
];

After conversion (with patch) - you can see that the databases defined in the "replica" target have updated successfully, and there are no db connection errors:

🇺🇸United States scott_earnest

Updates rolled into issue fork.

Unfortunately, PHPUnit is throwing an error, need to look into this:
https://git.drupalcode.org/issue/mimemail-3155325/-/jobs/2799865
---- Drupal\Tests\mimemail\Functional\MimeMailWebTest ----

Otherwise this works for me in Drupal 10 and Drupal 11.

🇺🇸United States scott_earnest

scott_earnest made their first commit to this issue’s fork.

🇺🇸United States scott_earnest

The above commit fixes the issue in #195 - and seems to function.
Still seeing an issue - when a field has a condition to be required, you can see the star, but the field can be submitted without actually filling out the field. Also, if the field is marked as required but not visible, the validation is still wanting that field to be filled out. So still in "needs work" but perhaps a little better.

🇺🇸United States scott_earnest

Error on admin configuration screen (/admin/config/system/siteimprove)
ArgumentCountError: Too few arguments to function Drupal\Core\Form\ConfigFormBase::__construct(), 1 passed in /app/web/modules/contrib/siteimprove/src/Form/SettingsForm.php on line 79 and exactly 2 expected in Drupal\Core\Form\ConfigFormBase->__construct() (line 43 of core/lib/Drupal/Core/Form/ConfigFormBase.php).

Per change record, ConfigFormBase now expects 2nd parameter for TypedConfigManagerInterface
- https://www.drupal.org/node/3404140

Above commit adds this new field.

🇺🇸United States scott_earnest

scott_earnest made their first commit to this issue’s fork.

🇺🇸United States scott_earnest

Tested on:
- Drupal 11.0.1
- PHP 8.3.10
- MySQL 8.0.19

Verified:
- Verifications configuration can be viewed (/admin/config/search/verifications)
- Verifications can be added of types:
- meta tag
- file upload
- verification file & contents
- Verification meta tags and files are produced as expected.

RTBC +1

🇺🇸United States scott_earnest

Ran into error trying to upload file:
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "file_validate_extensions" plugin does not exist.

file_validate_extensions is deprecated, see:
https://api.drupal.org/api/drupal/core%21modules%21file%21file.module/fu...
https://www.drupal.org/node/3363700

fixed on commit above:
https://git.drupalcode.org/project/site_verify/-/merge_requests/2/diffs?...

🇺🇸United States scott_earnest

Tested on:
- Drupal 11.0.1
- PHP 8.3.10
- MySQL 8.0.19

Verified:
- Plugin configuration can be viewed (/admin/structure/plugin)
- Plugin details can be viewed (/admin/structure/plugin/block for example)
- Plugin reference field can be added to an entity
- Plugin reference field can be viewed

Automated tests needed updates (committed on the issue fork).
These tests will fail, but this is not new on this patch (issues exist in current version):
- tests/src/Unit/Plugin/Plugin/PluginSelector/AdvancedPluginSelectorBaseTest.php
- tests/src/Functional/PluginSelectorTest.php

RTBC +1

🇺🇸United States scott_earnest

Testing this also throws errors - I think this is because the interface to add a field has been changed by Drupal (around 10.3?) to where a content admin must select the field type first, then move on to the next screen.
- tests/src/Functional/PluginSelectorTest.php

Again, these deprecations exist in the current version so does not seem to be something that is introduced by this patch.

🇺🇸United States scott_earnest

Testing this file throws errors, but they exist on the current version:
- tests/src/Unit/PluginType/PluginTypeManagerTest.php

The following classes are undefined, and not pulled in through the project composer.json file:

use org\bovigo\vfs\vfsStream;
use org\bovigo\vfs\vfsStreamDirectory;
use org\bovigo\vfs\vfsStreamWrapper;

... so taking no action here as it pertains to D11 upgrade.

🇺🇸United States scott_earnest

Seeing error running phpunit test on file:
- tests/src/Unit/Plugin/Plugin/PluginSelector/AdvancedPluginSelectorBaseTest.php

https://git.drupalcode.org/issue/plugin-3433935/-/jobs/2636647

    Drupal\Tests\plugin\Unit\Plugin\Plugin\PluginSelector\AdvancedPluginSelectorBaseTest::testbuildMultipleAvailablePlugins
    PHPUnit\Framework\MockObject\MethodCannotBeConfiguredException: Trying to
    configure method "buildPluginForm" which cannot be configured because it
    does not exist, has not been specified, is final, or is static

https://www.drupal.org/project/drupal/issues/3130606
"setMethods()" is deprecated, and should be using "onlyMethods()"

🇺🇸United States scott_earnest

scott_earnest made their first commit to this issue’s fork.

🇺🇸United States scott_earnest

Related ticket - to fix phpunit deprecation:

https://www.drupal.org/project/jsonlog/issues/3471285 🐛 JsonLogTest deprecation with PHPUnit version 10 Active

🇺🇸United States scott_earnest

Moving to reviewed. I will create new ticket for unit test deprecation warning.

RTBC +1

🇺🇸United States scott_earnest

Tested on:
- Drupal 11.0.1
- PHP 8.3.10
- MySQL 8.0.19

Verified
- Admin configuration page can be accessed at /admin/config/development/logging
- Admin configuration page can be edited and saved w/o issue
- Function to "Log test entry" works as expected, and creates an entry in the log file:

{"@timestamp":"2024-08-30T13:57:16.923Z","@version":1,"message":"Dummy message","message_id":"5f7965e45736__drupal1166d1cfbce15ce5.58857930","site_id":"5f7965e45736__drupal11","canonical":"","method":"POST","tags":"","type":"drupal","subtype":"200 - TEST entry","severity":"Warning","request_uri":"https:\/\/d11.lndo.site\/admin\/config\/development\/logging","referer":"","uid":1,"username":"","client_ip":"172.20.0.2","link":"","code":0,"trunc":"","context":[]}

Automated test results
- Functional/JsonLogConfigFormTest.php: OK (1 test, 12 assertions)
- Unit/JsonLogDataTest.php: OK (22 tests, 55 assertions)
- Unit/JsonLogTest.php: Tests: 11, Assertions: 37, Deprecations: 2.
- Unit/MockTranslation.php: ERROR Translation.php does not extend PHPUnit\Framework\TestCase

Looking into the 2 unit tests that were not successful.

🇺🇸United States scott_earnest

removed extra line EOF on the info file - this causes issues with the D.O. packing script and can lead to fail to apply patch. same MR should be all good thanks for the help!

🇺🇸United States scott_earnest

Having trouble applying the MR, testing issue w/EOF on info.

🇺🇸United States scott_earnest

note to maintainers - this commit drops support for Drupal 9 and Drupal 10.2 or less:

https://git.drupalcode.org/issue/adminimal_theme-3460890/-/commit/44327e...

thank you

🇺🇸United States scott_earnest

Tested on:
- Drupal 11.0.1
- PHP 8.3.10
- MySQL 8.0.19

Verified:
- Adminimal can be installed in "Appearance"
- Adminimal can be used as the Administration theme
- Adminimal settings can be view and saved (/admin/appearance/settings/adminimal_theme)
- Creating and using the custom "adminimal-custom.css" overrides works as expected
- Layout is the same as the Drupal 10 version
- Tested different admin pages including node edit, block layout, and media library with no regressions seen

RTBC +1

🇺🇸United States scott_earnest

new patch can be found by clicking on the "plain diff" link in the issue fork section:
https://git.drupalcode.org/project/adminimal_theme/-/merge_requests/15.diff

cc @jonasanne

🇺🇸United States scott_earnest

Tested on:
- Drupal 11.0.1
- PHP 8.3.10
- MySQL 8.0.19

Verified:
- Seven can be installed in "Appearance"
- Seven can be used as the Administration theme
- Seven settings can be view and saved (/admin/appearance/settings/seven)
- Layout is the same as the Drupal 10 version
- Image widget appears as expected

Automated test ran with success
- tests/src/Functional/SevenLayoutBuilderTest.php
- OK (1 test, 13 assertions)

🇺🇸United States scott_earnest

After patch:

Marking as ready for review. Thank you!

🇺🇸United States scott_earnest

Gitlab:

Locally:

🇺🇸United States scott_earnest

@jcisio - this has been merged into the dev branch 4.x - thank you for your contribution!

It will be part of the next release.

Marking as "Fixed" on Version "4.x-dev"

🇺🇸United States scott_earnest

Unfortunately, I think the pipelines build errors are preventing from merging in the issue fork. Will patch directly on the dev branch and push.

🇺🇸United States scott_earnest

Greetings @primus-

You should be able to update the "Uswds accordion section" paragraph type to allow "Text Field" and "USWDS Accordion". This will allow you to nest the accordions:

/admin/structure/paragraphs_type/uswds_accordion_section/fields/paragraph.uswds_accordion_section.field_accordion_section_body

🇺🇸United States scott_earnest

Looking into this I can see there is a commit from back in 2017 with message "disabled SLO for the time being", that comments out the contents of the saml_sp_user_logout() function (implementing hook_user_logout().

So it seems that this functionality may have existed at one point, but perhaps got deprecated? The function hook name does not correspond to the submodule it is called from saml_sp_drupal_login, so does not seem to be implemented because of that as well.

Either way, it would appear that this ticket needs work.

Looking at the simplesamlphp_auth module, they also use the same hook, so seems to be the correct way to implement it. Perhaps there is something in that module that can help with this one:
https://git.drupalcode.org/project/simplesamlphp_auth/-/blob/8.x-3.x/sim...

Perhaps this is something that can be configured on the IDP side? If this were the case the perhaps just a documentation update.

If the logout cannot be achieved in a reasonable amount of time, then perhaps the setting for "Single Log Out" in the admin UI should be hidden or deprecated.

🇺🇸United States scott_earnest

@jcisio - thank you for this patch.

I have verified that it works as intended with the following configuration:
- Drupal 10.3.2
- PHP 8.3
- SAML Service Provider (saml_sp) 4.3.3
- SAML Drupal Login (saml_sp_drupal_login) 4.3.3

TESTING RESULTS

Prior to patch:

After applying patch:

Verified:

  • "You are not allowed..." warning message
  • Submit button is disabled
  • Form cannot be submitted using keyboard "enter/return"

Moving to RTBC

🇺🇸United States scott_earnest

Upon testing I am finding that the dialog only appears once, and clicking "yes" will make it go away as fixed by the patch.

Unfortunately I am seeing a deprecation. I find that if you don't click anything, nothing happens. The expected behavior would be to auto-logout as defined by the "Timeout padding" setting.

Moving back to "needs work". I will try to help with a solve thank you for the patch.

🇺🇸United States scott_earnest

I think this is a duplicate of this issue. Also I see the issue could be related to Drupal core 10.2 -> 10.3 update.

https://www.drupal.org/project/autologout/issues/3466843 🐛 AutologoutManager should use type from psr/log package Closed: duplicate

🇺🇸United States scott_earnest

Hello @eiriksm-

I am sorry, but I am unable to reproduce this error. Would you be able to provide some additional information for steps to reproduce?

Also I see that the MR is against the 8.x-1.x branch, which I see is the default, but this ticket is linked to 2.x.

I am testing with these steps, prior to enabling the patch
- disable the "dblog" module
- enable the "monolog" module, following "Quickstart" instructions
- enabled autologout and tested
- verified I got a log entry in the "debug-*.log" file, similar to:
autologout.INFO: Session automatically closed for test by autologout.

I tested both versions 8.x-1.x on Drupal 9, and version 2.x on Drupal 10, and was not able to reproduce the error.

Thank you kindly for your help with this ticket.

🇺🇸United States scott_earnest

Tested on Drupal Version:
11.0.0-rc1

Tested all displays and all seem to work fine! Open to more tests or perhaps the bot will return...

🇺🇸United States scott_earnest

Got the Gitlab CI working, but unfortunately trying to fill all the stylelint errors introduced a bunch of deprecations.
Without the time at the moment to troubleshoot, decided to disable this check in the CI.

Did similar with esLint.

This build passes with no warnings:
https://git.drupalcode.org/project/media_video_micromodal/-/pipelines/24...

🇺🇸United States scott_earnest

I was not quite sure how to move the ticket along, but I would agree a patch should be in the acquia/cohesion issue queue, but also the code snippet is necessary for this fix to work. Perhaps moving to the correct issue queue is a start, then may just adding documentation regarding the custom hook?

🇺🇸United States scott_earnest

@vflirt - thank you for this fix - it works great and I would like to give RTBC+1 for us with:

- Drupal 10.1.8
- PHP 8.2.16
- Cohesion/Site Studio 7.2.1

Some important notes for those applying the fix:

1. The patch works in conjunction with the code snippet in Comment 2 🐛 Editing a clone before saving changes the source node. Active

2. The patch is for the package acquia/cohesion, not quick_node_clone, so if you are having issues applying with composer make sure the correct package is specified.

"acquia/cohesion": {
    "3272297 - Editing a clone before saving changes the source node (quick_node_clone)": "https://www.drupal.org/files/issues/2022-11-22/editing_a_clone_before_saving_changes_the_source_node-3272297-5.patch"
}
🇺🇸United States scott_earnest

We have been copying our database to a lower environment, then running a database update, then sql-sync to local.

This command will clear the problem records:
drush @stage sql-query "DELETE FROM redirect_404 WHERE path LIKE '%my-duplicate-key-redirect%'"

Or we have dropped the PRIMARY KEY, this also works:
drush @stage sql-query 'ALTER TABLE redirect_404 DROP PRIMARY KEY'

This is our current workaround.

🇺🇸United States scott_earnest

Thank you @sandzel for the patch!

This is working for us:
- Drupal 10.1.6
- Acquia DAM Version: 1.0.11
- 8.1.18

RTBC +1

Production build 0.71.5 2024