Looking at 🐛 YAML discovery does not take theme inheritance into account Active I don't think we should be logging here. I think this is too low level place to log - we've benefitted from this code prior to 🐛 YAML discovery does not take theme inheritance into account Active dealing with this situation and I think we should continue to. There definitely should be a decent comment as to what is happening here.
Discussed #8 with @phenaproxima we agreed that in this situation the event is offering benefits - the event object collects skips and validates them. This type of collecting and validating with an object has not been done with hooks yet and there is no need for this to be first.
Also discussed with @longwave ahile back and we agreed to backport this to 11.1.x because while this is not a bug in core it helps fix a bug possible when you have trash installed - which Drupal CMS does.
Committed and pushed 51ae03891c1 to 11.x and c813f52a5be to 11.1.x. Thanks!
Committed and pushed ac347da742f to 11.x and 379ebd46fcc to 11.1.x. Thanks!
We could backport this 10.4.x and 10.5.x if we choose.
I have a couple of concerns about the approach. We're moving code into the block_content module that depends on workspaces even though block_content does not depend on workspaces - are we sure this is the correct thing to do?
@rgpublic system_requirements is how admin/reports/status is generated - I agree we should only do this when not in the CLI. We already have quite a few requirements that only run when if ($phase === 'runtime' && PHP_SAPI !== 'cli') {
The current implementation in system_requirements is problematic because of adding the file to public... and then removing it. That's not free. Not sure I have a better idea yet. And maybe that means punting the system_requirement part to a follow-up is a good idea.
We've got test coverage and I've discussed the access implications with @daniel.bosen and we agreed on this approach.
alexpott → created an issue.
We've got test coverage and this is working at expected.
@dries did you have to make any changes to the dries theme? Did code move about? It's really odd that on 11.0.x the theme was being found and added correctly and then updating the code let to it not being found but reinstalling it fixed the problem.
@greggles great idea only changing the .htaccess in the files directory. This move made me think 2 things.
1. We need a update function to rewrite existing sites .htaccess files I think
2. What about sites which use nginx or another server... what would be neat is something on the status report that confirms this protection is working. That way we could point to documentation on how to set this up.
Getting this into 11.2.x nice and early - before it is open. So it has a long time in the up-coming branch.
Committed 3004fc8 and pushed to 11.x. Thanks!
Tested this on a multiple entities on a large site. It works well. Before this change it would just hang. As above only manual t4ests are possible here really.
As this is a refactor and only occurs on large sites this in not really testable by adding new tests. We already have test coverage of the paragraph updating functionality so at least we can be sure we've not broken that.
alexpott → created an issue.
Okay let's keep on with dummydb but instead of extending the abstract connection class lets extend another db driver so we don't have to think about / maintain so much stuff we're not using here.
I definitely think this issue should be only about setting a CSP for SVG files - the bit where we add a default CSP for all other private files needs discussion and additional thought.
The kernel failure occurred because before we had two services in services.yml files implementing \Drupal\views\Plugin\views\query\CastSqlInterface and now we have 1 - ie. the one in views.services.yml so now it errors. The best fix is to add the missing alias. The fact this is in a plugin directory is odd because it is a backend overridable service and it only ever accessed in real runtime code by doing \Drupal::service('views.cast_sql') so it's not a plugin at all. Anyhow that's not something this issue needs to resolve.
Relatedly and conversely whe you edit simple configuration that's not supported - for example workspaces.settings on admin/config/workflow/workspaces/settings when you are in a workspace it'll save and make immediate changes to the live config, which feels at best quite inconsistent.
Committed and pushed c9c3966566e to 11.x and 63be3c483d3 to 11.1.x. Thanks!
So the test passes but when I do the steps it fails. The test passes locally for me too. I've changed the test to use the standard install profile and make literally the same changes I do so I'm quite confused as to why this is happening. Need to debug the revert process locally and see what is happening.
@greggles I retested - it was a random fail. So all green again.
How about adding a driver_test driver to core/modules/system/tests/modules/driver_test can be a copy of system/tests/modules/driver_test/src/Driver/Database/DriverTestMysql with the driver name changed to driver_test... less to maintain but the same amount of code coverage.
I disagree with the assessment that they are separate things. Sometimes they are and sometimes they are not. That's why I gave the PHP extension checking example in my comment. That is checked regardless of phase.
If we look at a contrib example:
/**
* Implements hook_requirements().
*/
function commerce_requirements($phase) {
$requirements = [];
if ($phase == 'install' || $phase == 'runtime') {
if (!extension_loaded('bcmath')) {
$requirements['commerce_bcmath'] = [
'title' => t('BC Math'),
'description' => t('Commerce requires the BC Math PHP extension.'),
'severity' => REQUIREMENT_ERROR,
];
}
}
return $requirements;
}
This shows how things are flawed currently - the BC Math extension is just as much required during the update phase as it is in the install and runtime phase - imagine an update function doing something with data relying on the BC math depending code for example.
I think #21 is worth considering before we add yet more test code to maintain.
This happens for any config entity type that you edit why a workspace is active. I think we need to prevent access or something neater than thrown an exception.
alexpott → created an issue.
Apart from config changes that affect the db schema, which were "solved" by not being allowed in wse_config, the biggest challenge of that module (and any per-workspace-config solution in general), is the multitude of caching layers stacked on top of various config/services/plugin managers/etc. that have to be made workspace-aware.
I've been pondering this quite a bit today and looking at the caching stuff in wse_config and thinking about @wim leers's config override POC from a few months ago and I'm wondering if we can using the config override system to help us here. I.e. not actually use config overrides but make it look like a config override is active for an config we've changed in the workspace. Then we might not have to do so many cache shenanigans in wse_config.
At the moment, the list of config that can be edited in a workspace is based on a "manual" allowlist, but a proper/long-term solution IMO would be to introduce something along the lines of
FullyValidatableconstraint from core that would work similarly to\Drupal\Core\Form\WorkspaceSafeFormInterface.
At the moment for config entities it is opt-in and uses an allow list approach. For simple configuration it's the other way around it has a list to exclude (including system.site). This mixture of approaches confused me for a bit.
I found this issue via the issues the create hook_*_requirements(). I'm not sure about the focus of the API design here which feels mostly centred on how can we possibly convert hook_requirements to OOP and not around what's the best API for gathering requirements from a module. This issue has some of the problems with moving to OOP - i.e. dependency injection in a fragile and changing environment being fraught and that's good to see. And I agree that anything that makes system_requirements easier to manage would be great but I'm not sure the answer is split it up into 3 separate things and have massive duplication.
I think we need to make it possible for the same requirements code to be run on install, update and runtime phases so, for example, we don't have three different hook implementations that do the PHP extension checking that's done currently in system_requirements().
Wrt to dependencies the following code makes me fear for what we're going to have deal with:
// Test if configuration files and directory are writable.
if ($phase == 'runtime') {
$conf_errors = [];
// Find the site path. Kernel service is not always available at this point,
// but is preferred, when available.
if (\Drupal::hasService('kernel')) {
$site_path = \Drupal::getContainer()->getParameter('site.path');
}
else {
$site_path = DrupalKernel::findSitePath(Request::createFromGlobals());
}
The idea of the phase being runtime and there's not always a kernel service is mind boggling.
I'm not sure about the decisions that are creating this API. It feels like the question is how can we make this an OOP hook - and not what is the correct API for gathering requirements during install, update and runtime. The solution being advocated for in this and the other issues feels just asking ourselves what is possible with OOP hooks - and not what it is the correct API for gathering requirements from modules.
The issue summary here is missing the why we should do this. What is wrong with the old system of using $phase. If the answer is the hook system updates to be OOP - then I'm not sure that we're thinking about this the right way around.
Another way to recreate this problem with only core modules is to make taxonomy dependent on workspaces... and then install minimal and then taxonomy... kaboom! Works fine on 11.1.x.
alexpott → created an issue.
I think the key decision is
we're fairly certain that we should use Workspaces as the entity storage model for this, regardless of how much of the Workspaces UI we end up using.
Given this feels nearly certain I think we need to make that certain and go all in a model that allows us to make configuration changes as part of workspace.
What confuses this issue a bit are the revert requirements on 🌱 Decide on approach to use for storing past revisions of XB config entities Active . When a workspace is applied to a live site it'll create a single revision for each content entity that is changed regardless of the number of revisions for that content entity in the workspace. I think this makes sense - each workspace is viewed as a single set of changes to make to the live site. I think config should behave the same. Unfortunately this is not the way wse_config is working at the moment. It looks like it is creating a content entity for each configuration edited in a workspace and when it is published it changes the live configuration. If you visit the WSE Config listing page (admin/config/development/configuration/config-content) you will see a list entities changed in each workspace and these changes don't really relate to the change that's happened to the live workspace. All WSE Config content entities are listed here regardless of the workspace you are on. You don't see a nice history like you do for node/n/revisions
One thing that makes me very unsure about workspaces and configuration is the possibility that the configuration change when you publish a workspace will affect more than just the content in the workspace being being published and the same goes for a revert. I think this breaks the mental model of what a workspace encapsulates.
This is a critical bug because it prevent correct installation of modules that provide fields for entity types they do not provide themselves.
Yeah this is a core bug. We should only be processing field storage updates if the provider of the entity type is not in the module list.
This is happening because wse_config and workspaces are being installed at the same time. The wse_config module depends on workspaces (via the wse module), so workspaces is installed first. This means that when we loop round do entity updates we try and create a reference field on the wse_config entity type before the entity type actually exists. Because we process the workspaces in this loop before wse_config.
I think this is a core bug. I think we should process entity type creations before we do any field adding.
alexpott → created an issue.
I think we should park the field creation in workspace discussion till we've work out what we're going to do. Creating fields that are not represented in live configuration is going to be fraught. I don't want to think about how the system report is going to deal with validating the entity schema definitions, for example.
I think I should have given more context to the conflict resolution remark - the moment you edit content in a workspace you can no longer edit it in the live site. The system prevents you from doing this. It's more conflict prevention than resolution I guess. The point is that the same it not true for configuration when you have wse_config installed.
I think we should be fixing the PHPStan errors in the new class we're adding - I can't see why we'd add these errors to the baseline.
We need to add a comment about why we're suppressing errors here.
I think using the config override system is a bit fraught for revisions because how it will interact with config translations which also use the override system. It's not a neat fit. On the other hand using config overrides at least makes the cache situation simpler because overrides are part of the config cache key.
A neat thing about using content entities to hold the future and past revisions of config is that we don't have to build a UI to manage that.
@nikolay shapovalov I committed a quick follow-up to fix this.
Fixing link
Committed 7f54341 and pushed to 11.x. Thanks!
This seems like a good idea. Less special casing core FTW.
Added some comments to the MR to address.
@amateescu we only know that it is an override once the views service exists. This is about sites without views installed. The MySQL service is not tagged.
Fixed the MR.
alexpott → changed the visibility of the branch 3493595-the-mysql.views.castsql-service to hidden.
I removed the test because we do not require tests for constructor changes like this.
Committed 6728ca1 and pushed to 11.x. Thanks!
@daffie the changes look great. Will commit once this is rtbc again
@daffie the event listener turned out great! Think we need to make a small adjustment for non core tests and trigger a deprecation until Drupal 12. See MR for suggestion for how to do this.
alexpott → made their first commit to this issue’s fork.
@amateescu but without views installed we have no idea that mysql.views.cast_sql exists to override views.cast_sql
alexpott → created an issue.
alexpott → created an issue.
alexpott → created an issue.
Workspaces should allow to revert a workspace after it has been published. Unfortunately this is currently broken for wse_config but it could be fixed.
From the issue summary it feels that you are expecting reverts to take place outside of a workspace - I think we should update the issue summary with a user journey that explains how and why a user would want a revert capability.
@catch actually system.site configuration is ignored by wse_config see https://git.drupalcode.org/project/wse/-/blame/2.0.x/modules/wse_config/...
Here's a video of wse_config in action - put it on youtube because it's a big file - https://youtu.be/m4CsXtI3420
Key findings so far:
- You can stage config changes with wse_config
- Revert appears to be broken
- Conflict resolution is not in place. If you change a config on live before it is staged it will become automatically enabled on stage. Once a config change is made in the workspace it will apply over the top of any config changes made on live when the workspace is published.
One thing to think about here is what happens with entities with multiple paragraphs linking to the same entity - how many revisions are they going to get?
Committed and pushed a44cf1ef008 to 11.x and 5b1a9182e9f to 11.1.x and 0a5101f68df to 10.5.x and d7f5dbc09cd to 10.4.x. Thanks!
As this unblocks Drupal CMS and after discussion with @longwave agreed to put this in 11.1.x - since it applies also put it in 10.4 and 10.5
Dupe of 📌 Rewrite to use core's Default Content API Needs work
@tim.plunkett wants to have a look at this before we merge it.
alexpott → made their first commit to this issue’s fork.
This looks great - a simple fix that allows core's default content to import content created by the default content module plus the layout builder patch from ✨ Add a Normalizer and Denormalizer to support Layout Builder RTBC
Added some comments to the MR.
alexpott → created an issue.
alexpott → created an issue.
alexpott → created an issue.
@jonathan1055 I agree we shouldn't be testing 11.x by default but we should update to 11.x.0-beta as soon as it is available as this surfaces problems with a release once we've made a commitment to the API. Not doing this makes it more likely that real sites break.
larowlan → credited alexpott → .
xjm → credited alexpott → .
alexpott → created an issue.
Discussed with @daniel.bosen and we think this is ready. I've just pinged @Berdir because I want to understand why the following happens with ERR... I have a node with a paragraph field - it happens to not be translatable. I load the load, setNewRevision(TRUE) and save it… why do I get new revisions of the paragraph items even though they have not changed?
alexpott → created an issue.
There's a 3.x version with Drupal 11 compatibility - see 📌 Drupal 11 compatibility for config_selector Fixed
alexpott → created an issue.