Committed and pushed 37d8ea5cb4 to 11.x and e1902fee88 to 11.0.x and eeca1aec09 to 10.4.x and 2664526bdb to 10.3.x. Thanks!
From the verbose we see:
[0;37m[service:selenium__standalone-chrome-selenium-standalone-chrome-selenium] 2024-07-27T08:33:55.799363620Z 08:33:55.799 DEBUG [DriverService.start] - Starting driver at /usr/bin/chromedriver with [--port=6662, --log-path=/builds/chromedriver.log][0;m
[0;37m[service:selenium__standalone-chrome-selenium-standalone-chrome-selenium] 2024-07-27T08:33:55.806694897Z 08:33:55.806 DEBUG [UrlChecker.waitUntilAvailable] - Waiting for [http://localhost:6662/status][0;m
[0;37m[service:selenium__standalone-chrome-selenium-standalone-chrome-selenium] 2024-07-27T08:33:55.807341040Z 08:33:55.807 DEBUG [UrlChecker.lambda$waitUntilAvailable$1] - Polling http://localhost:6662/status[0;m
[0;37m[service:selenium__standalone-chrome-selenium-standalone-chrome-selenium] 2024-07-27T08:33:55.807568985Z 08:33:55.807 DEBUG [ExternalProcess$Builder.lambda$start$0] - completed to copy the output of process 64[0;m
[0;37m[service:selenium__standalone-chrome-selenium-standalone-chrome-selenium] 2024-07-27T08:33:55.810179587Z 08:33:55.809 WARN [DriverServiceSessionFactory.apply] - Error while creating session with the driver service. Stopping driver service: Driver server process died prematurely.[0;m
So the key part seems to be that selenium fails to start up chromedriver - it'd be great to see what is in /builds/chromedriver.log
alexpott → created an issue.
Can this be backported to 10.3.x to aid with backporting the W3C change?
Can this be backport all the way to 10.x - there's no harm and it'll help backporting the W3C issue.
alexpott → changed the visibility of the branch 3463534-legacy-for-now to hidden.
The tests are failing in the right way - they are missing the OTEL_COLLECTOR env variable that is set in the schedule. So the revert to use chromedriver will work fine. Created 📌 Move performance test to selenium Active to fix this.
alexpott → created an issue.
Committed and pushed a01c36ccaf to 11.x and 277352a450 to 11.0.x and 9e55a96be2 to 10.4.x and b6d1fffef3 to 10.3.x. Thanks!
alexpott → created an issue.
Hmmm... I've used the same string locally and it runs... added more detail to the runs and the skip is happening due to:
The test wasn't able to connect to your webdriver instance. For more information read core/tests/README.md.
The original message while starting Mink: Could not open connection: Could not start a new session. Error while creating session with the driver service. Stopping driver service: Driver server process died prematurely.
Build info: version: '4.22.0', revision: 'c5f3146703'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.1.66-93.164.amzn2023.x86_64', java.version: '17.0.11'
Driver info: driver.version: unknown
Host info: host: '3ad2463107b7', ip: '192.168.160.3'
Build info: version: '4.22.0', revision: 'c5f3146703'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.1.66-93.164.amzn2023.x86_64', java.version: '17.0.11'
Driver info: driver.version: unknown
Feel like I'm missing something simple.
alexpott → created an issue.
Yeah this is the arguments... let's fix it.
This can now be attempted - I think we should deprecate this in 11.2.0
📌 Use selenium/standalone-chrome instead of our chromedriver image Needs work has landed
I think we do want to get this in 10.4.x and maybe even get the test base class changes into 10.3.x and 11.x so that projects can move to W3C driver testing early. I'll open when I get round to making the MR for 10.4.x
Do we need to update https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... → to?
One thing that makes me hesitate on this change though is that we need a plan for how this information is going to be used. If it's just just going to sit in the issue summary then not many people are going to see it and the value in writing it is minimal. I realise the issue summary says that this will help the initiative and that's a good thing, but I think the process needs to be a bit more apparent. For example, as this is a new section I think the issue summary template could link to initiative and be more explicit about what is expected - other people are not going to know what to do and whether it is relevant. On a large number of issues, i.e. nearly all bug fixes, this section should be removed or N/a'd.
For some more context this was changed in PHP 7.1.0 - see https://bugs.php.net/bug.php?id=71897
alexpott → created an issue.
I think we need to apply some validation to the name parameter... let's make it a valid PHP function name. Excludes things like unicode and means it has to start with a letter which will be good for yaml keying.
We should add test coverage for the attribute name validation to core/tests/Drupal/Tests/Core/Config/Action
+1 to closing this as works as designed. Maybe some docs are missing. The fix in the MR would change settings.php for all test environments which would not be the correct fix here - but yeah there is no fix to do here :)
I've tweaked the way W3C mode is set in nightwatch to negate the need for an extra environment. I realised that the selenium environment was missing the merge of chrome args etc... and changing how this is set up seemed best.
I've also update the Cr to reflect the test configuration changes someone would need to make to use W3C compliant browser testing.
Added testing performance follow-up as requested.
alexpott → created an issue.
Added a comment to the MR.
Crediting @thejimbirch - thanks for the initial work on this one!
Committed 1c21864 and pushed to 11.x. Thanks!
Given where we are at in the 11.0.0 release cycle we can't commit this to 11.0.0 and have to target 11.1.0 instead. I've updated the deprecation messages and tests on commit.
diff --git a/core/modules/block/src/Entity/Block.php b/core/modules/block/src/Entity/Block.php
index 2effc409b7..68582e48f0 100644
--- a/core/modules/block/src/Entity/Block.php
+++ b/core/modules/block/src/Entity/Block.php
@@ -349,7 +349,7 @@ public function preSave(EntityStorageInterface $storage) {
parent::preSave($storage);
if (!is_int($this->weight)) {
- @trigger_error('Saving a block with a non-integer weight is deprecated in drupal:11.0.0 and removed in drupal:12.0.0. See https://www.drupal.org/node/3462474', E_USER_DEPRECATED);
+ @trigger_error('Saving a block with a non-integer weight is deprecated in drupal:11.1.0 and removed in drupal:12.0.0. See https://www.drupal.org/node/3462474', E_USER_DEPRECATED);
$this->setWeight((int) $this->weight);
}
diff --git a/core/modules/block/tests/src/Kernel/BlockValidationTest.php b/core/modules/block/tests/src/Kernel/BlockValidationTest.php
index 9a1a2412d6..2456e7fb57 100644
--- a/core/modules/block/tests/src/Kernel/BlockValidationTest.php
+++ b/core/modules/block/tests/src/Kernel/BlockValidationTest.php
@@ -176,7 +176,7 @@ public function testWeightValidation(): void {
public function testWeightCannotBeNull(): void {
$this->entity->set('weight', NULL);
$this->assertNull($this->entity->getWeight());
- $this->expectDeprecation('Saving a block with a non-integer weight is deprecated in drupal:11.0.0 and removed in drupal:12.0.0. See https://www.drupal.org/node/3462474');
+ $this->expectDeprecation('Saving a block with a non-integer weight is deprecated in drupal:11.1.0 and removed in drupal:12.0.0. See https://www.drupal.org/node/3462474');
$this->entity->save();
}
diff --git a/core/modules/search/search.module b/core/modules/search/search.module
index 8d5b1e330d..0944c53dcf 100644
--- a/core/modules/search/search.module
+++ b/core/modules/search/search.module
@@ -428,7 +428,7 @@ function search_block_presave(BlockInterface $block) {
if ($block->getPluginId() === 'search_form_block') {
$settings = $block->get('settings');
if ($settings['page_id'] === '') {
- @trigger_error('Saving a search block with an empty page ID is deprecated in drupal:11.0.0 and removed in drupal:12.0.0. To use the default search page, use NULL. See https://www.drupal.org/node/3463132', E_USER_DEPRECATED);
+ @trigger_error('Saving a search block with an empty page ID is deprecated in drupal:11.1.0 and removed in drupal:12.0.0. To use the default search page, use NULL. See https://www.drupal.org/node/3463132', E_USER_DEPRECATED);
$settings['page_id'] = NULL;
$block->set('settings', $settings);
}
This is definitely an issue. The views in the module are shipping with a permission that does not exist. I think that having permissions for for price rules, price lists and price list items is unnecessary. Atm:
- price rule uses Drupal\commerce\EntityPermissionProvider
- price list uses Drupal\commerce\EntityPermissionProvider
- price list item uses "administer commerce_price_rule"
Not sure of the correct structure though. It feels like price list item should use something from price list and not price rule.
This is looking great - love the work @mandclu and @phenaproxima are doing.
Posted some comments on the MR - most of which are are tiny nits.
I've run core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipeRegressionTest.php --filter testMultipleReplacements on both old chromedriver and selenium and the test fails with 2000 and the fix from
🐛
big_pipe sometimes fails to load blocks
Active
reverted so reducing to 2000 seems fine and we don't need a comment. That was useful to explain why we have reduced it but will be meaningless in the future.
I've answered the rest of @longwave's feedback on the MR - but back to needs review. I think we need to add a nightwatch section to the MR.
One thing for reviewers to not is the move from 2 runners to 4 runners for the JS testing job. This is because testing on selenium is slower. There is not much we can do about this as far as I can see and the benefits of actually testing against current browsers and not maintaining our own chromedriver image are large.
It's with sad regret about test autoloading that I rtbc this. Nice work @phenaproxima within the confines of our system.
Created the follow-ups.
I think this is ready for review.
alexpott → created an issue.
alexpott → created an issue.
alexpott → created an issue.
Marking this as a duplicate of 📌 Use selenium/standalone-chrome instead of our chromedriver image Needs work - @justafish is creditted on that issue already.
Answered questions in the issue summary and update MR to reflect the answers.
alexpott → changed the visibility of the branch 3421202-on-top-of-3240792 to hidden.
@RandalV any chance you can review the MR I just posted and RTBC it - it would be great to fix this problem rather than maintain the patch on this issue.
Added some more review comments. Nice work on fixing up the int-ness of weight @phenaproxima
I don't think that config validation issues shoudl just in a type widening for used values. Weight is always an integer once a block has been saved because config casts the value.
Config schema casting on save is why this is never actually a NULL once a block is saved. I'm not sure that this should be a separate issue from 📌 Fix Block config entity type config schema violations: weight, property Postponed
The 3.x branch will the Drupal 11 compatible branch - fixed in 📌 Make Drupal 10.2 the minimum for 3.x branch and allow (add test on) 11.x Needs review
The 3.x branch will the Drupal 11 compatible branch - fixed in 📌 Make Drupal 10.2 the minimum for 3.x branch and allow (add test on) 11.x Needs review
The 3.x branch will the Drupal 11 compatible branch - fixed in 📌 Make Drupal 10.2 the minimum for 3.x branch and allow (add test on) 11.x Needs review
Added an MR to backport this to 10.x using the annotation for before instead of the attribute.
The MR takes us to only 7 functional test methods being skipped that actually take longer than 0.00 seconds! Down from over hundred...
I think this is ready for review.
Better title...
alexpott → created an issue.
To be fair you can see the skipped tests in the report on gitlab - see https://git.drupalcode.org/project/drupal/-/pipelines/227006/test_report
Lolz previous me pointed out that PHPUnit would skip if something required by the test is not available - see #3187577-11: FunctionalJavascript tests should fail when ChromeDriver is not running → - I guess that comment is still correct. But it would be great if somehow the reason things are skipped was actually listed in the test output.
@andypost that's not the case... they were skipping every test :D we should fail instead of skip when a WebDriverTestBase test fails to connect to a browser. Once I fixed the webdriver json string we can see that the tests are about the same speed - see https://git.drupalcode.org/project/drupal/-/pipelines/227038
Also Firefox has a different approach when elements are not in the viewport and you do a click - see https://stackoverflow.com/questions/44777053/selenium-movetargetoutofbou... - I think we should address this in our webdriver in the same way that webdriver-classic-driver does - see https://github.com/minkphp/webdriver-classic-driver/blob/73ad0b6ce21cf69...
Fixed up issue summary and title to be inline with current MR. Also added more to remaining tasks for things that we need to decide.
alexpott → changed the visibility of the branch 3421202-use-seleniumstandalone-chrome-instead to hidden.
My plan was to run a limited subset tests using non-W3C mode. while we still support it - i.e. until Drupal 12 and then in Drupal 12 we should only support W3C testing.
I think we should also add a manual job to run tests using Firefox.
Backported to 10.3.x because recipes are not API and the whole thing is not stable yet anyway!
Committed and pushed 9f3ee1ff55 to 11.x and 3fd22d6b62 to 11.0.x and f734ef114e to 10.4.x and 22972e3764 to 10.3.x. Thanks!
I added some review comments to the MR. I agree with doing this on fork because I think until we have the ability to actually get user input we should not solidify the API by adding to core.
The issue title here is wrong because we're not getting user input here at all.
@Prashant.c I think the task of applying over an standard profile install is an interest one. I wonder if it is possible to apply over a site installed from the standard recipe.
I think what @Prashant.c's testing implies is that we need to add a re-application test to \Drupal\FunctionalTests\Core\Recipe\StandardRecipeTest and maybe to \Drupal\FunctionalTests\Core\Recipe\StandardRecipeInstallTest... it could be in \Drupal\Tests\standard\Traits\StandardTestTrait::testStandard...
This looks like really nice work. It is create to see createIfNotExists being used to make recipes re-usable. This is why I added it. Nice one @phenaproxima
The new actions look good too.
I think we need an example of a useful dialog that can be achieved without javascript... I'm not sure about the use case for a dialog that does not really do anything. And in the case of using a form with the action dialog again I'm left pondering the use-case. See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog for why I'm asking use-case questions.
I think this issue should have considered contrib. There are usages there, especially in commerce sites. Especially in commerce - including code that assumes it is set. See http://codcontrib.hank.vps-private.net/search?text=country.default&filen... - especially the default country resolver.
Actually hmmm - looking at contrib usages we have a different story.... see http://codcontrib.hank.vps-private.net/search?text=country.default&filen...
Actually I think that result calls into question the decision to remove the setting from the installer. What this does mean is that I'm not sure we should do this work.
An example update test would be \Drupal\Tests\locale\Functional\LocalesLocationAddIndexUpdateTest - note you can use the bare db file instead of the filled and it should go in core/modules/system/tests/src/Functional/Update
Plus this issue is going to need to remove the setting from the form that allows you to set it.
@wells if you select the people to credit on the issue, save and then use the merge button on the issue everything is as you would expect. If you use the merge button on gitlab then you need to bring fix up the commit message.
@wells nah it's a commit... no way to change that git history now without doing silly things. I think credit on the issue is fine.
Added some more review comments to address.
@borisson_ yeah I think for things like page.403 / page.404 - null meaning not configured is okay. I'm especially +1 on this use of null if you can configure an empty value in the UI and the setting is not used a label. For me labels should not be nullable as they are highly likely to me passed to a string function and TranslatableMarkup won't work with NULLs.
For me the situations is something like this:
- Labels / translatables should always be strings in config. If an empty string does not make sense then it should have a not blank or not blank after site install (as necessary) - for example system.site:slogan should allow blanks but syste.site:name should not allow blanks after install.
- Config data that is optional (not labels) should consider allowing a NULL for an empty value, especially if an empty string makes no sense. This is the system.site:page.403/4 case.
- Config data that requires a non blank value after install should not allow a NULL - this is the system.site:uuid case.
I've opened 📌 Add NotBlankAfterInstall constraint and use it for system.date:timezone.default Active to explore the NotBlankAfterInstall idea.
alexpott → created an issue.
alexpott → created an issue.
Came up with a new idea on #3443432-23: Add validation constraints to system.site → ... we can add a new constraint NotBlankAfterInstall that allows empty strings during install but not after. I think this will reduce the need for nullable and means that we can leave values as strings and not have type flip-flopping.
I think this issue points to us needing a new constraint which is something like NotBlankAfterInstall - so a very can be blank during site install but once the site is up and running it can not be. This is true for uuid, name and email.
I also think that none of these should be nullable. PHP 8 makes swapping between NULLs and strings hard.
Looking at the system.site example from 📌 Add validation constraints to system.site Needs work I do not think any of the values should be nullable. I think the labels should always be strings. A empty string a slogan, for example, is better than a NULL for me. The UUID property is interesting because it is assigned during the installer but I do not think we should be making it NULLABLE because a NULL is not a valid value ever.
alexpott → created an issue.