- 🇺🇸United States smustgrave
For the 1 comment in the MR.
Also seems like will need a CR.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇳🇿New Zealand quietone
The MR does add 'development mode' to the new development page.
- 🇬🇧United Kingdom catch
My thoughts with the UI were that people can either directly set it in a local services.yml (where twig debug already is), or that assuming we add this to the new development page, it could be set via that (with key/value in the middle like the other toggles).
- 🇳🇿New Zealand quietone
In slack catch mentioned the UI, so I changed this to container parameter. Is this preferred?
- 🇬🇧United Kingdom scott_euser
Just confirming that I tested changes from @VladimirAus and continue to have the error described in #175
- 🇳🇿New Zealand quietone
I talked to catch about the needs for tests and we agree that manual testing is fine. If someone wants to write a test, that is OK, but not a requirement.
That leaves the question of whether this should be in settings or a container parameter. There was also the idea of having it in services.yml. See #80.
I did manual testing and added screenshots. They may need to be changed depending on where the value is stored.
- 🇺🇸United States smustgrave
If I’m looking at the code correctly if the setting is TRUE there will be a warning on the status page
- 🇳🇿New Zealand quietone
Tests for the status report is fine though.
There is no mention of the status report in the issue summary. The first mention is in #93.
What is expected to be on the status page?
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
+1 to not being too specific about what this affects. We'll end up having to change that all the time or risk it getting stale.
- 🇬🇧United Kingdom catch
This would be set in settings.local.php (or equivalent) so it won't end up in config or version control.
We will need to add test coverage when we actually use this in places, but that's not introduced in this issue. Tests for the status report is fine though.
As discussed already in the issue, 100% agree to this being only a bool, then it's up to developers where they actually set it on or off according to workflow.
Two questions:
1. Should this definitely be a setting in settings.php and not a container parameter?
2. I'm not sure we should say this is unrelated to settings like javascript aggregation and twig debug - we may well end up changing the behaviour of those when it's set in follow-up issues. And even if we didn't do that in core, contrib could.
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
We don't test that many settings for their values, do we? I know we have some tests for showing up on the status report, but not for the actual settings value.
- First commit to issue fork.
- 🇺🇸United States smustgrave
Probably checking that the setting does what described. And message appears on the status report page
- 🇳🇿New Zealand quietone
@smustgrave, can you elaborate on what a test would be testing?
- 🇺🇸United States smustgrave
Could we add test coverage for this?
Only concern is I know the new development settings was meant to not be captured in config/repo. This change can kinda undo that
- 🇳🇿New Zealand quietone
I couldn't run the pipeline either. Then I saw that some of the linting had failed, so I reran those and then the tests ran. There were lots of failures but there are no test reports. Odd.
On the good side the pipeline and reporting is working as expected now.
- 🇺🇸United States loze Los Angeles
I am seeing the same as @malcomio and this patch fixes it for me as well.
Drupal 10.4.2 - 🇬🇧United Kingdom malcomio
See 🐛 Nested field groups improperly display as "Disable" in the admin UI Active for details of an issue affecting the field_group module - the core patch seems to fix that for me.
@jsimonis - how are you applying the patch, and what are the failures that you see when trying to apply the patch?
https://vazcell.com/blog/how-apply-patch-drupal-9-and-drupal-10-composer is a good explanation of how to do this.
- 🇺🇸United States loze Los Angeles
The patch in #149 applies and works as expected for me in drupal 10.4
thanks! - 🇺🇸United States kentr Durango, CO
Hid the original MR because the new MR builds on it.
- 🇯🇴Jordan Rajab Natshah Jordan
Attached a static
drupal-core--2025-02-06--2741877--mr-11137.patch
file from this point of MR11137
witch applies to both Drupal 10.4.x
To be used with Composer Patches - @rajab-natshah opened merge request.