- ๐ฆ๐บAustralia acbramley
This looks awesome! A massive improvement over the current state.
Looks like the CR needs updating as the screenshot no longer matches what's been implemented.
- ๐ฆ๐บAustralia sime Canberra
I've preferred settings.php overrides for nearly 10 years, and they have felt like a second class citizen to contrib solutions for all that time. I support the sentiment of #267, #268, etc. Massive step forward.
I tested the same things as #272. Since I last looked at issue I see there's a twig template for the message which was a nice surprise! So I also tested that I could override that, because for now I would do that as a standard practice to explain to site builders where the values are being overridden.
- ๐บ๐ธUnited States smustgrave
Retested this one again on the performance page and the message definitely helps for me.
Verified the link is correct
Verified the anchor links for the configuration correctly go to the right fields and clicking them moves focus to the configuration.Think this would be a good improvement
- ๐ฌ๐งUnited Kingdom AaronMcHale Edinburgh, Scotland
Usability review was done in #255, I think I just forgot to remove the tag, sorry for any confusion.
- ๐ฎ๐นItaly mondrake ๐ฎ๐น
Once this is in, the main CR for PHPUnit 10 will need adjustment.
- ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
This looks fantastic - nice work @mondrake
Raising this to major as it fixes a regression in Drupal 11. The regression is the ability to use paratest - which works great on D10. I think with this change and the move of deprecation reporting to PHPUnit it might be finally possible to consider removing run-tests.sh in favour of paratest which would be quite amazing.
- ๐ฎ๐นItaly mondrake ๐ฎ๐น
Let's see if I can please everyone with the following:
1. Let's have xml parameters for output directory and verbose, with defaults. Verbose = true
2. Let's keep the two env variables as overrides of the xml parameters, if they exist (i.e. no more deprecation)This way we combine both worlds, and on CI we do not have to tamper with the xml file.
On that.
- ๐บ๐ธUnited States greg.1.anderson
@alexpott: you are correct, the summary is out of date. It should say ANSI and TRADITIONAL rather than listing all of the individual modes that Drupal used to set. If we did https://www.drupal.org/project/drupal/issues/3261236 โ , then Drupal would use individual mode values again. I don't know if that issue is a good idea or not, but if it is, it wold be better to do this one first.
There is very little motivation for individual users to set these mode values in most cases. Right now, Pantheon is tracking an issue were sites migrated from other systems where they used to run on Mysql sometimes suffer performance penalties when the same SQL queries run on Mariadb. Allowing control over individual modes would give ISPs more flexibility, e.g. we could set a mode for a group of sites, and individual sites could later alter individual modes further if necessary.
Another concern I have is that providing an API for this means that we have to track changes and do deprecations for values when the underlying modes change in the database API.
That's fair. Maybe it's better to go back to strings of values instead of providing a class with constants.
- ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
I've applied this fix to #3452524: Fix tests on 11.x due to PHPUnit 10 โ and it works great. See https://git.drupalcode.org/project/distributions_recipes/-/jobs/1822778 for a successful run using paratest.
- ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
++ this works great and fixes running Drupal tests using paratest. We should definitely do this. Thanks @mondrake.
- ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
The issue summary states:
However, it's an all or nothing override. Connection::open does this:
$connection_options['init_commands'] += [ 'sql_mode' => "SET sql_mode = 'ANSI,STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,ONLY_FULL_GROUP_BY'", ];
so either I override the whole of that string, or none of it.
But given we're not merging the defaults I don't understand how the new code is better - according to the way the issue summary lays out the improvement. Given the lack of times that changing this is necessary are we sure that all this extra code and processing on every bootstrap is really worth the additional maintenance and effort to change existing set ups?
@greg.1.anderson @joachim are there any other benefits here?
Another concern I have is that providing an API for this means that we have to track changes and do deprecations for values when the underlying modes change in the database API.