🇺🇦Ukraine @pingwin4eg

Thanks @sourabhsisodia_ that's a great start.

I looked through the code and I see an alt field, but not the title. I think it should be there too.

Would be good to see some screenshots of the result before I or anyone interested will get to manual testing.

pingwin4eg → created an issue.

pingwin4eg → created an issue.

pingwin4eg → created an issue.

I've been thinking about this, and now I see 2 reasons to actually switch from guzzle to a "wrapper in wrapper" way:

1. As http_client service can be swapped with another one, it is unclear if another service would use same option names as guzzle.
2. If I understand correctly, guzzle fetches the whole response at once, so using streams could be better in terms of performance (time, memory).

Fix me if I'm wrong.

$this->config['stream'] = true

This is incorrect approach since this way guzzle uses stream wrapper, leading to an indefinite recursion: remote stream wrapper -> guzzle -> remote stream wrapper...

I did some investigation regarding context options, and I think it's possible to provide the method, headers and probably some other options from the stream context (stream_context_get_options($this->context)) to guzzle's request() $options, but I'm not sure about all of them (not even sure it's possible, but I'm not an expert in this field).

There's a separate issue for remote_stream_wrapper:7.x-1.x-dev - #2522056: Use the native PHP stream wrapper if available. → . It tries to achieve the same (or at least similar) goal using a different approach - "by wrapping the wrapper" if I understand it correctly. Maybe that is a better way, but as I see from comments - still not everything is possible.

This issue summary needs update to be more general and clear. But considering my observations I don't know whether the issue should try to work out all the context options, or not, and in which way.

PhpUnit previous major (Drupal: 10.3.6, PHP 8.1.30) and next minor (Drupal: 11.1.0-dev, PHP 8.3.13) both pass on all tests except one for HttpMimeTypeGuesserTest, but that should be fixed by 📌 Fix mime-type guesser for Drupal >= 10.3.2 Active .

I'm also going to manually test this MR in one of my projects in the near future. And I'm thinking (not sure) - whether the auto-testing for file functions (fopen, etc.) is needed.

pingwin4eg → changed the visibility of the branch project-update-bot-only to hidden.

Working on removing constructor from stream wrapper.

pingwin4eg → changed the visibility of the branch 2.x to hidden.

Phpunit passed for Drupal 10.3.6 and PHP 8.1.30 - https://git.drupalcode.org/issue/remote_stream_wrapper-3486396/-/jobs/32... . And failed for D11 (expected, because the 2.x branch doesn't ready for D11 yet).

pingwin4eg → created an issue.

It seems the 8.x-1.x branch is not supported anymore, since it's not in the table of releases on the main project page.

The 2.x branch is D10-compatible.

The 2.x branch is already D10-compatible, omitting this issue.

Re: MR !12

Stream wrapper must not have a constructor. Because this breaks fopen() and alike functions:

[error]  ArgumentCountError: Too few arguments to function Drupal\remote_stream_wrapper\StreamWrapper\HttpStreamWrapper::__construct(), 0 passed and exactly 1 expected in Drupal\remote_stream_wrapper\StreamWrapper\HttpStreamWrapper->__construct() (line 53 of .../web/modules/contrib/remote_stream_wrapper/src/StreamWrapper/HttpStreamWrapper.php) #0 [internal function]: Drupal\remote_stream_wrapper\StreamWrapper\HttpStreamWrapper->__construct()

Look at core implementations of StreamWrapperInterface - they all use static calls to \Drupal::service(...) or similar.

I get another error in 1.6-rc1:

ArgumentCountError: Too few arguments to function Drupal\Core\Form\ConfigFormBase::__construct(), 1 passed in .../web/modules/contrib/linked_field/src/Form/ConfigForm.php on line 36 and exactly 2 expected in Drupal\Core\Form\ConfigFormBase->__construct() (line 43 of core/lib/Drupal/Core/Form/ConfigFormBase.php).

pingwin4eg → created an issue.

Shouldn't the change record be in the draft status at this stage? I see it's published, but the issue isn't committed yet, so nothing described in it works.

And looking at the form code, it's not only with sqlite. Other DB types, drush also have versions hardcoded in strings. The correct implementation is with the PHP version check.

Ah, so the actual minimum is different. Well, then yes, the text is wrong. The version value in it should be replaced with a placeholder.

I've patched the module with project-update-bot-only changes (those only bumping versions) in one of my projects and updated Drupal core to 11. It runs OK on cron - a migration is being scheduled and items are imported without issues. Auto-tests are green. It would be great to see these changes committed.

pingwin4eg → created an issue.

I see the same warning, but on the "GTranslate Settings" page, right after installing the module and visiting the page.

GTranslate module 3.0.1
Drupal 10.2.5
PHP 8.1.28

Warning: foreach() argument must be of type array|object, string given in Drupal\Core\Render\Element\Checkboxes::valueCallback() (line 113 of core/lib/Drupal/Core/Render/Element/Checkboxes.php).

Drupal\Core\Render\Element\Checkboxes::valueCallback(Array, , Object)
call_user_func_array(Array, Array) (Line: 1285)
Drupal\Core\Form\FormBuilder->handleInputElement('gtranslate_admin', Array, Object) (Line: 1006)
Drupal\Core\Form\FormBuilder->doBuildForm('gtranslate_admin', Array, Object) (Line: 1076)
Drupal\Core\Form\FormBuilder->doBuildForm('gtranslate_admin', Array, Object) (Line: 1076)
Drupal\Core\Form\FormBuilder->doBuildForm('gtranslate_admin', Array, Object) (Line: 579)
Drupal\Core\Form\FormBuilder->processForm('gtranslate_admin', Array, Object) (Line: 325)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 627)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 704)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

pingwin4eg → made their first commit to this issue’s fork.

Is it safe to use class names like that in recent comments?

What if an extension declaring the hook is not dependent on those other extensions and classes may be missing?

Is it possible to use just extension names instead?

hestenet → credited pingwin4eg → .

Needs work / doesn't pass tests:
- https://git.drupalcode.org/project/styleswitcher/-/jobs/2339518
- https://git.drupalcode.org/project/styleswitcher/-/jobs/2339511

pingwin4eg → made their first commit to this issue’s fork.

pingwin4eg → created an issue.

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.