- ๐บ๐ธUnited States smustgrave
For the 1 comment in the MR.
Also seems like will need a CR.
- ๐ณ๐ฟ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?
- ๐ณ๐ฟ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.
- ๐บ๐ธ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.