🇺🇦Ukraine @pingwin4eg

pingwin4eg → created an issue.

Not sure whether it's a bug or not.

pingwin4eg → created an issue.

pingwin4eg → created an issue.

Hi all. Will this issue be further worked on? I mean the CKEditor support. Should I apply the patch or one of merge requests?

pingwin4eg → created an issue.

Drush error appears even without a table prefix, removing it from IS. It was caused by xdebug.

I found out that the SQLite driver uses database attachment concept instead of actual table prefixes. Not sure whether this is OK or not, and why it was implemented this way.

If it's OK, then the settings.php documentation needs to mention that, the prefix is better be prepended to a DB file name or appended to a base name before the file extension, and reported errors need to be fixed.

In D11 the situation is a bit different, but the main problem is the same. Updated IS.

pingwin4eg → created an issue.

Trying to run php web/core/scripts/drupal server in D11 I get another error:

The service file "core/core.services.yml" is not valid.

The patch applies and fixes the error.

The only remaining task is probably to write a test case. I'm not sure about the directory structure in the testing environment, is there a "web" (or another docroot) directory?

pingwin4eg → created an issue.

If this is still an issue, feel free to re-open, and work on a patch.

I'm trying to understand the case where this can be useful. Why a cookie is not enough for your case?

I'm trying to understand the case where this option can be useful.

I have a site with the multisite system.
So I need to have domain setting for cookies.

So currently the module sets a cookie for a domain which a visitor is viewing when switching styles. Do you want a site visitor to click a link in one of multisites, but view another one in a chosen style?

Hi @vishal.kadam . First of all, thanks for reporting this.

Unfortunately, I can't reproduce the bug in a "fresh" installation. You should provide more context.

As for your patch, it just prevents the error in console, but doesn't fix it. You click a link in a block, but a style is not changing, if I understand correctly.

I suggest you to try the latest dev version of the module in your installation, and report back. If the error still exists, please provide more details - which browser, drupal core version, other installed modules, anything that might help reproduce it.

@radheymkumar What's the purpose of the patch you uploaded? If I understand correctly, there are only changes to field_collection.api.php and tests (from the patch #11).

Auto-tests pass. Manual testing remains.

Thanks @fjgarlin. I did as you said, ran pipeline in the issue fork repo manually after commit, and it worked. The log - https://git.drupalcode.org/issue/styleswitcher-3403145/-/jobs/489497

pingwin4eg → created an issue.

Thanks @nitin_lama for working on this.

I'll continue from here.

The Nightwatch testing fail seems unrelated.

DrupalPractice code sniffing standard revealed more errors and warnings to fix.

quietone → credited pingwin4eg → .

AFAIK themes don't have a .module file, these should be changed to .theme in this doc.

pingwin4eg → created an issue.

The bug seems to appear only when $config['system.performance']['css']['preprocess'] = TRUE. When it is so, the library's media attribute is changed from the custom value to 'all', so \Drupal\styleswitcher\StyleswitcherElementInfoAlter::preRenderHtmlTag() doesn't set the id attribute to the link element.

Opened 🐛 Fix dynamic style switching in D10 Active for the described bug.

Thanks again everyone who worked on compatibility fixes.

pingwin4eg → created an issue.

Manual testing showed a bug - "dynamic" style switching doesn't work, you can see a style applied only after a page refresh or after opening another page. This misbehavior happens only in Drupal 10 on a real site, but not under the test (which is weird). So basically that is not an incompatibility with D10.

I'm going to commit the changes in the patch, and start a new issue for the described bug.

I think the best would be to have a template instead of styleswitcher_admin_style_overview and a pre_render callback instead of theme_styleswitcher_admin_styles_table.

Thank you Tim, and thank you DA! We really appreciate this.

Then it's a separate issue (and requires steps/explanation how it happens). This one is for compatibility fixes.

Does it happen only in D10?

Closed MR, because it was not full.

1. +++ b/js/styleswitcher.js
   @@ -56,14 +56,16 @@
   -    // Update the cookie first.
   -    Drupal.styleSwitcher.cookie(style.name);
   +    if (style) {
   +      // Update the cookie first.
   +      Drupal.styleSwitcher.cookie(style.name);
    
   -    // Now switch the stylesheet. Path is absolute URL with scheme.
   -    $('#styleswitcher-css').attr('href', style.path);
   +      // Now switch the stylesheet. Path is absolute URL with scheme.
   +      $('#styleswitcher-css').attr('href', style.path);
    
   -    // Cosmetic changes.
   -    Drupal.styleSwitcher.switchActiveLink(style.name);
   +      // Cosmetic changes.
   +      Drupal.styleSwitcher.switchActiveLink(style.name);
   +    }
      };
   

   Is this necessary?

2. +++ b/styleswitcher.libraries.yml
   @@ -6,7 +6,7 @@ styleswitcher:
   -    - core/jquery.once
   +    - core/once
   

   jQuery is still used in styleswitcher.js.

Interdiffs would save reviewers' time, by the way.

There's one more use of the '#theme' => 'styleswitcher_admin_style_overview' in StyleswitcherAdmin::buildForm(), which is not worked out in the patch.

Reviewing.

I also encountered the "container was serialized" issue with a migration test in Style Switcher module with D 9.5.11, PHP 8.1, PHPUnits 8.5.31 (local) and 9.5.28 (DrupalCI):

1) Drupal\Tests\styleswitcher\Kernel\Migrate\d7\MigrateVariablesTest::testVariablesMigration
PHPUnit\Framework\Exception: PHP Fatal error:  Uncaught AssertionError: The container was serialized. in /var/www/html/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php:99
Stack trace:
#0 /var/www/html/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php(99): assert(false, 'The container w...')
#1 [internal function]: Drupal\Core\DependencyInjection\ContainerBuilder->__sleep()
#2 Standard input code(89): serialize(Array)
#3 Standard input code(123): __phpunit_run_isolated_test()
#4 {main}
  thrown in /var/www/html/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php on line 99

When I switched PHP to 7.3 locally I saw the actual error:

1) Drupal\Tests\styleswitcher\Kernel\Migrate\d7\MigrateVariablesTest::testVariablesMigration
PHPUnit\Framework\Exception: PHP Fatal error:  Uncaught LogicException: The database connection is not serializable. This probably means you are serializing an object that has an indirect reference to the database connection. Adjust your code so that is not necessary. Alternatively, look at DependencySerializationTrait as a temporary solution. in /app/core/lib/Drupal/Core/Database/Connection.php:2042
Stack trace:
#0 [internal function]: Drupal\Core\Database\Connection->__sleep()
#1 Standard input code(80): serialize(Array)
#2 Standard input code(112): __phpunit_run_isolated_test()
#3 {main}
  thrown in /app/core/lib/Drupal/Core/Database/Connection.php on line 2042

The workaround was to call $this->startCollectingMessages(); before $this->executeMigrations(...).

Thank you folks for your work on this issue!

Tests are failing, so changing status to NW. Going to review and test everything myself.

I closed the issue because the module was designed to switch a style without page reloading.

+ I would say this is still critical to support Ukraine until the aggressor is fully defeated. Please, return the banner.

The patch needs a re-roll against the contrib module.

I get this too after an upgrade to D10. The code worked in D9.

Found a duplicate. Not sure which one to close. That one was created earlier, but not in the ldap project.

Found a duplicate. Not sure which one to close. This one was created earlier, but not in the ldap project.

I could only find 2 such calls, both in the same file.

pingwin4eg → created an issue.

So this patch belongs to ldap project.

pingwin4eg → created an issue.

The fix from the merge request works - the default private directory (private://feeds/in_progress/#) is used instead of the temporary one.

For those who still see the error with the tmp file, check also other fetcher classes. For example, in our client's project there are also the feeds_http_auth_fetcher module with it's own fetcher, and the custom one, and existing feed entities use all 3. So I had to change them too similar to the http fetcher.

For example, it should not make any changes to configuration objects or entities.

Are there alternative ways to change configs and entities? For example, if there's a need to add fields to an entity type and fill them with values during a module install.

In a website development it's a usual thing that new custom modules are installed in all envs except local through the configuration import.

Patch #47 applies and works perfectly with 9.4.8.