Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to detect a pattern from default theme having same id as the pattern in the base theme #304

Closed
msnassar opened this issue Sep 3, 2020 · 5 comments

Comments

@msnassar
Copy link

msnassar commented Sep 3, 2020

Steps to reproduce:
Create a pattern in your base theme.
Copy the previously created pattern to your sub theme (default theme).
ui patterns library detects the pattern defined in your base theme instead of the one in the default theme.

I think a pattern defined in the default theme (having same id as a pattern in the base theme) should be the one detected. As it overrides the pattern in the base theme!

@vever001
Copy link

vever001 commented Sep 4, 2020

+1 on this!

By looking at the code, it looks like this behavior was done on purpose.
Not sure if that's the case or why this would be desirable.
So unless there is a strong reason not to, IMO the sub-themes should be able to easily override any pattern from the base theme.
In other words, respect the usual flow of overrides: modules, base themes, sub-themes.

And the logic could be simplified to:

protected function getDirectories() {
  return $this->moduleHandler->getModuleDirectories() + $this->themeHandler->getThemeDirectories();
}

Same logic as \Drupal\Core\Layout\LayoutPluginManager and \Drupal\breakpoint\BreakpointManager.

@donquixote
Copy link

I made an interesting observation:
Without the patch, the yml definition from the base theme is used, but the template from the sub-theme.
The reason is that each pattern is also a theme hook, and the sub-theme template overrides the base theme template via the theme system.

With the patch, both the yml definition and the template from the sub-theme are used.

After removing the yml in the sub-theme, the yml from the base theme and the template from the sub-theme are used - which is good, I assume.

@donquixote
Copy link

Module-provided patterns vs theme-provided patterns

The solution proposed here fixes the order of base theme vs sub-theme.
However, what about module-provided patterns? Should they be overridden by theme-provided patterns?
For the template this should already work through the theme layer, but for the yml definition it won't, because module directories are added last.

Pattern inheritance compatibility validation?

Also, I wonder if there should be any validation to check the compatibility of the theme-provided pattern vs the module-provided pattern.
Do we need something like LSP (Liskov substitution principle) for patterns?

@kp77
Copy link

kp77 commented Dec 9, 2020

I'd like to add to this problem that I also had errors during site install with existing config, when using patterns in layout builder, because during install the default theme is stark.

@vever001 's suggestion would also fix that problem, so I created a follow up PR that uses that method: #318

@mika2na
Copy link

mika2na commented Sep 23, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants