🇪🇸Spain @penyaskito

penyaskito → created an issue. See original summary → .

This looks great overall, but we lack tests for the update path and some minor issues.

Sadly we don't have any php tests for the component library, so should be fine (but sad) not adding that coverage here.

Thanks!

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

OP was about setting gin as active when it isn't.
MR is about setting other theme as active when it isn't, and gin not breaking the page.

#19 $fieldTypeProp is defined as FieldTypePropExpression|ReferenceFieldTypePropExpression|FieldTypeObjectPropsExpression and the other 2 are already covered in the previous ifs. I'd remove the

    // @phpstan-ignore-next-line instanceof.alwaysTrue
    if ($storable_prop_shape->fieldTypeProp instanceof FieldTypeObjectPropsExpression) {

with an assertion

\assert(storable_prop_shape->fieldTypeProp instanceof FieldTypeObjectPropsExpression);

For the record, this happened to me last time I tried with enums, you cannot swap from a list_string back to a regular string.
Might have been fixed in the meantime, but worth to verify too in the context of this issue.

(Sadly) this is still very relevant.

penyaskito → created an issue.

👆🏼 Now I cannot reproduce #31 consistently 😿. Have a MR created for Gin tho that fixes the issue @ 🐛 Gin does not render properly if set as active theme from a custom theme negotiator Active tho

But that raises a valid question: is this reproducible with 6.x and the given patch? Note, that also requires Drupal core 11.x-dev

I couldn't generate this scenario because of some other canvas bug making it incompatible with the Gin 6.x branch.
I'm still debugging that one, but I'm quite convinced this won't be an issue given 📌 Clean-up Gin PHP Active .

This is quite a blocker for Drupal CMS/Canvas, so happy to create a follow-up to explore that, but it shouldn't be as critical as this is now.

The attached MR is fixing the problem for canvas.

The included test tbh is useless for testing the issue. I could reproduce this before outside of canvas just by trying to save the Gin settings form, but haven't been able to reproduce it consistently either. Happy to keep it or remove it as your will.

This is easily reproduced inside canvas / Drupal CMS 2.x-alpha1, but didn't find yet a way to trigger the problem if not in conjunction with such infrastructure.

Merged !35

While we are here let's run npm audit too and prevent the release if there are criticals?
IICR you were doing that manually.

The test failure is because we are not recursing any object that we could have (and it could be another object embedding another object, etc).

This is not a bug in SDC but in the component, prop = NULL is not the same than omitting a prop. which wouldn't trigger any error. But I agree this is terrible DX when using components inside other components because of twig, and should be solved.

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

LGTM, checked with Phéna in Vienna sprints

And this is the project browser issue for that: 📌 Project Browser blocks should be derived regardless of whether they're enabled Active

Crediting @aboros because he noticed in Vienna sprints room.

penyaskito → created an issue.

penyaskito → created an issue.

Agree, the local menus / routes should, but the blocks themselves shouldn't filter by active.

I think the sequence is:

- Recipe modules are installed, Canvas is installed (PB default settings are drupalorg_jsonapi, recipes)
- \Drupal\project_browser\Plugin\Derivative\BlockDeriver returns the two blocks, so components are generated.
- Recipe config action runs, we set enabled_sources to recommended, drupalorg_jsonapi. New component is generated for: recommended.
- Try to disable recommended: the block deriver is not returning this one anymore, so disabling fails (and a new version is generated because of the fallback).

penyaskito → created an issue.

Back-porting 📌 Clean-up Gin PHP Active might be a solution. But that requires a core patch. Wondering if this is reproducible at all in gin 6.x

Right now the tab is "Disabled & incompatible components". But disabled aren't listed there, so makes it even more confusing.

Disclaimer: I didn't check if the list on the IS was exhaustive.

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

huh? the simplest test ever fails, I cannot save the Gin settings form.

From the top of my head this can affect paragraphs too.
Fix is trivial, and even has tests! Test-only jobs are failing as expected.

Drupal Canvas 1.0.0-RC1 → released! 🎉

And now while testing this, it breaks the minute I enable "Enable sticky action buttons (Beta) (New)" on a new installation, and I save the theme settings form, without canvas being involved.
So no, we cannot fix this here. It has to be gin.

Discussed with @effulgensia and, while we want to fix the root cause of this, this shouldn't hold the RC releases.

So:

1) Let's merge !171 solution. Is good enough, and as hooks implementations are considered @internal APIs we can freely remove that when we have a proper fix in place.
2) We can keep this open afterwards and investigate the proper issue here or in follow-ups.

+1 I guess it's fine not having a test for this.

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

Can't merge, this is one of those "Requires 3 approvals from Code Owners."

IMHO this is ready for reviews now.

A snippet that I used for generating + inspecting the dump:

https://git.drupalcode.org/-/snippets/248

#24.4: Added dynamic prop to content template → bd5cb038

+ Simplified tests in multiple commits. It's definitely more compact, not sure if more maintainable tho, I see pros and cons.

penyaskito → created an issue.

Some questions, but preemptively marking RTBC as Wim is the shapes' expert.

This is still relevant. We can do that by using assertions if we want to avoid refactoring the setHeader().

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

Great catch!

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

There's a new tagged version of gitlab_templates so default-ref will use 11.2.5, therefore we can omit CORE_STABLE

done: https://git.drupalcode.org/project/gitlab_templates/-/tags
changelog: https://git.drupalcode.org/project/gitlab_templates/-/blob/main/CHANGELO...

New debugging session:

\Drupal\Core\Form\FormBuilder::buildForm does:

    // If the incoming input contains a form_build_id, we'll check the cache for
    // a copy of the form in question. If it's there, we don't have to rebuild
    // the form to proceed. In addition, if there is stored form_state data from
    // a previous step, we'll retrieve it so it can be passed on to the form
    // processing code.
    $check_cache = isset($input['form_id']) && $input['form_id'] == $form_id && !empty($input['form_build_id']);
    if ($check_cache) {
      $form = $this->getCache($input['form_build_id'], $form_state);
    }

In this case, the form has been already created, and the theme was gin, so the #after_build is added.
But when we retrieve it back from the cache, the current theme is canvas_stark, so gin.theme is not loaded and that produces the failure.

One way to proof that:

In \Drupal\canvas\Controller\EntityFormController::form, use

    $form_array = $this->formBuilder()->buildForm($form, $form_state);
    unset($form_array['form_build_id']); // This prevents the form being retrieved from the cache.

    return $form_array;

That fixes the issue, but sadly introduces another problem when you trying to remove the image by clicking the "x".

Re-opened #3539391-6: Gin does not render properly if set as active theme from a custom theme negotiator → .

The site WSODs in the opposite case, where Gin is the default admin theme but the negotiator switches to another theme.

This is the case we are exploring at 🐛 Media library broken on 11.2 with Gin 5 Active , so this is a Canvas blocker + Drupal CMS blocker.

We're in the process of preparing Gin to be moved into Core, replacing Claro.

All the more reason to fix this in Gin.

I could reproduce this both in Drupal CMS and outside of it.
The problem only happens when

Enable sticky action buttons (Beta) (New)

is enabled. This happens automatically if the core Navigation module is enabled:

    // Sets default to TRUE if setting is enabled.
    $sticky_action_buttons = $settings->get('sticky_action_buttons') ? TRUE : FALSE;

    // Always enable if navigation is active.
    if (_gin_module_is_active('navigation')) {
      $sticky_action_buttons = TRUE;
    }

This patch on Gin works:

diff --git a/src/GinContentFormHelper.php b/src/GinContentFormHelper.php
index 4983393b..e39eab18 100644
--- a/src/GinContentFormHelper.php
+++ b/src/GinContentFormHelper.php
@@ -159,7 +159,7 @@ class GinContentFormHelper implements ContainerInjectionInterface {
       // Attach library.
       $form['#attached']['library'][] = 'gin/more_actions';

-      $form['#after_build'][] = 'gin_form_after_build';
+      $form['#after_build'][] = [GinContentFormHelper::class, 'ginFormAfterBuild'];
     }

     // Remaining changes only apply to content forms.
@@ -247,6 +247,11 @@ class GinContentFormHelper implements ContainerInjectionInterface {

   }

+  public static function ginFormAfterBuild(array $form, FormStateInterface $form_state): array {
+    require_once __DIR__ . '/../gin.theme';
+    return gin_form_after_build($form, $form_state);
+  }
+
   /**
    * Sticky action buttons.

but this is yet another approach to the symptom, not the root cause. I can confirm the after_build is added BEFORE the theme is negotiated, as Wim suggested in #15.

Because FormBuilder::doBuildForm calls \Drupal\Core\Render\ElementInfoManager::getInfo, which triggers the negotiation via $this->themeManager->getActiveTheme().

So my favourite potential fix at the moment is just ensuring that the admin theme is loaded even when we use canvas_stark, so any hooks it might have added still work when we switch themes.

E.g.

diff --git a/src/Theme/CanvasThemeNegotiator.php b/src/Theme/CanvasThemeNegotiator.php
index 46e45c0e..656819de 100644
--- a/src/Theme/CanvasThemeNegotiator.php
+++ b/src/Theme/CanvasThemeNegotiator.php
@@ -7,6 +7,7 @@ namespace Drupal\canvas\Theme;
 use Drupal\Core\Config\ConfigFactoryInterface;
 use Drupal\Core\Routing\RouteMatchInterface;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
+use Drupal\Core\Theme\ThemeInitializationInterface;
 use Drupal\Core\Theme\ThemeNegotiatorInterface;
 use Symfony\Component\HttpFoundation\RequestStack;

@@ -51,11 +52,17 @@ final class CanvasThemeNegotiator implements ThemeNegotiatorInterface {
   public function determineActiveTheme(RouteMatchInterface $route_match) {
     $triggering_element_value = $this->requestStack->getCurrentRequest()?->get('_triggering_element_value');
     $still_in_media_library = $triggering_element_value !== (string) $this->t('Insert selected');
+    $admin_theme = $this->configFactory->get('system.theme')->get('admin');

     if ($this->requestStack->getCurrentRequest()?->query->has('use_admin_theme') && $still_in_media_library) {
-      return $this->configFactory->get('system.theme')->get('admin');
+      return $admin_theme;
     }
     $this->requestStack->getCurrentRequest()?->query->remove('use_admin_theme');
+
+    /** @var $theme_initialization ThemeInitializationInterface */
+    $theme_initialization = \Drupal::service(ThemeInitializationInterface::class);
+    $theme_initialization->initTheme($admin_theme);
+
     return 'canvas_stark';
   }

Thoughts?

Caching context per dashboard_list cache tag + user.permissions cache contexts should be enough.

See recording trying to reproduce the issue:

@mandclu That's exactly what I tried to reproduce and I couldn't.

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

I was going to move this to core for SDCs, when I realized this is already in core (see https://www.drupal.org/docs/develop/theming-drupal/using-single-director... → ).

I've been trying to reproduce this issue with no luck.

Using Gin as administrative theme, edit Page:
- se image to SEO settings from media library, no problem.
- Add a card component from canvas_sdc_test and select an image from the media library, it works.

Looking at the code I saw Navigation might be involved, so enabled it and repeated the steps above, no issues.

I'm on Gin 5.0.x (5bed7e4a), Canvas 1.x (a0625e92).

With Gin 6.x (adafdb38) I get an error site-wide even before installation:

AssertionError: CSS must be nested under a category. See https://www.drupal.org/node/2274843. in assert() (line 198 of core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php).

Can we update the issue summary with steps to reproduce?

CONTRIBUTING.md doesn't require CODEOWNERS approvals, not sure if that's a bug or a feature 🤪.

Merging this. It's an improvement, and worse case we can do follow-up MRs.

For full traceability: the MR fixing the add-on is at https://github.com/drupal-xb/ddev-drupal-xb-dev/pull/47 and was released as v0.0.24 today.

Suggested commit message (taking into account MRs in github for the ddev add-on itself):

            
              
              
              📌
              Contributing/setup instructions are broken
                Active
              
             chore: Fix CONTRIBUTION.md instructions after the rename

By: balsama
By: ultimike
By: penyaskito
By: cosmicdreams
By: vishalkhode

I just did a release for https://github.com/drupal-xb/ddev-drupal-xb-dev, still pending: fix the instructions here.

😍 love it!

LGTM

What kind of tests do this need?

There's \Drupal\Tests\user\Kernel\UserAccountFormFieldsTest, \Drupal\Tests\user\Functional\UserCreateTest, \Drupal\Tests\user\Functional\UserEditedOwnAccountTest, \Drupal\Tests\user\Functional\UserEditTest and a bunch more that are _still_ passing.

This is a plain refactor without any new functionality.
We just a need a bit of manual testing for sanity check (it's the account form after all).

If you are more specific we can add the test coverage you require, but I think this is quite well covered.

test-only run failing as expected: https://git.drupalcode.org/project/canvas/-/jobs/6754775

Related: ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active .

This is the infra that would make this easier. Navigation would benefit. Dashboard would benefit. Layout builder has layout_builder_restrictions as a workaround, but would benefit from this.
Now Drupal Canvas too :-(

This is ready. phpstan failing because HEAD is actually failing.

Created ✨ Add ThemeUninstallValidatorInterface similar to ModuleUninstallValidatorInterface Active

penyaskito → created an issue.

penyaskito → created an issue.

#12: maybe the negotiation is happening too late?

I'd look at that first, or we might be fixing a symptom and not the root cause.

#16: Close, that's what I needed.

Isn't this an issue in Gin itself? They shouldn't alter the content form if it's not the active theme?

This is ready for review. Assigning to Wim, but I think this needs @larowlan's input too.

Do we have proper test coverage? I'm not 100% sure.

LGTM but I guess we need a test.

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

thanks for the quick reviews.

The easiest fix probably is checking the provider in \Drupal\canvas\Plugin\Canvas\ComponentSource\SingleDirectoryComponent::isBroken, and check if it's a theme and not the actual default theme or one of its ancestors.

Created related (but imho less of a priority) 🐛 Component tree containing theme's SDCs triggers errors when the theme is not the default anymore Active

Created 🐛 Component tree containing theme's SDCs triggers errors when the theme is not the default anymore Active , didn't add to issue summary listing.

penyaskito → created an issue.

penyaskito → created an issue.