nicxvan → credited tim.plunkett → .
Nice work! Verified that the `initial preprocess` approach is the standard practice
tim.plunkett → made their first commit to this issue’s fork.
tim.plunkett → made their first commit to this issue’s fork.
xjm → credited tim.plunkett → .
xjm → credited tim.plunkett → .
This looks great, thanks @justafish!
I think this is changing the directory in which CSpell is run, it's now picking up playwright-report/index.html
Okay so I noticed something else interesting.
I was the MR author on
🐛
External AI Chatbot Functionality
Active
and Utkarsh and I were joking about how when I push to the branch, the tests pass, but when anyone else pushed we'd have "random fails". Except this Playwright one hasn't felt random.
When it's someone else pushing to the fork, the pipeline looks like this
https://git.drupalcode.org/issue/experience_builder-3522013/-/jobs/55488...
When I push to it, it looks like this:
https://git.drupalcode.org/project/experience_builder/-/jobs/5549674#L57
The directory structure is different! And mirrors the URL structure!
So in my test runs, there is a directory called `experience_builder` but for other test runs it's `experience_builder-3522013`
Basic test coverage was added in a recent commit, see here
tim.plunkett → created an issue.
Unable to mark my thread as resolved, but it is. Thanks @phenaproxima!
#28 I don't disagree, but the hidden-ness of the module makes me feel better about it.
Boldly marking this RTBC, barring the random fails from Cypress and Playwright, this has been passing and the feedback has been addressed.
All feedback should be addressed by now, excluding further explanation of the test plan (see third paragraph of #22, and 📌 [PP-1] Add test coverage for Experience builder AI submodule. Active . There's no written record of @lauriii signing off on postponing the testing, but I was on the Zoom call.
Leaving as major since this seems to be failing 50% of the time, but at least @mglaman has a good explanation for why this ever passed.
@justafish rightly points out that the fact that this EVER works is concerning, this should probably have some checks around it
tim.plunkett → created an issue.
phenaproxima → credited tim.plunkett → .
tim.plunkett → created an issue.
Merged !811 🎉
Merged !813 🎉
Thanks
Took me a bit to track down why Project Browser's Stylelint started failing (due to usage of rgba()) , there was no CR. Idk if that's worth a full CR but it would be nice.
tim.plunkett → made their first commit to this issue’s fork.
tim.plunkett → created an issue.
Adding credit
This change makes sense and the test coverage is clear (and fails without the fix)
thejimbirch → credited tim.plunkett → .
@mondrake should be credited wherever we end up fixing our Gitlab setup, when that happens (or in 📌 MissingGroupException: Missing group metadata with run-tests.sh in 11.2.0-dev Active )
tim.plunkett → created an issue.
New component!
penyaskito → credited tim.plunkett → .
tim.plunkett → created an issue.
+1
We can rename the file anyway we want, but it has to be separate because of the conditional loading while we are also supporting PHPStan 1 (as these errors are only reported in PHPStan 2)
Agreed we can add some docs if we pursue 794
Two new MRs (!793 and !794) to finish this issue.
Both of them fix a logic "bug" in SortHelper, there's two redundant checks.
Both of them bump our composer.json to the latest version of phpstan-drupal
793 takes the opinion that PHPStan knows best and that we don't need to enforce the types via assert, and removes the extraneous ones.
This has the side benefit of making the calling code actually ensure it is passing what it thinks it's passing, see the change to DrupalCore.php that actually enforces what's in the configuration
794 takes the opinion that while PHPStan might know best, that doubling up with asserts is okay too.
This codifies the doubled asserts in a new baseline file, that is only because PHPStan 1 (or maybe phpstan-drupal 1) aren't flagging this yet.
I slightly prefer 793 because it has less maintenance burden, but I'd be fine with 794.
Merging for now, the 2.0.4 PHPStan-Drupal release solved some but not all of the problems, and I want to unblock other MRs.
Merged !791 🎉, leaving open
tim.plunkett → created an issue.
This should include test coverage for `container`
Discussed this with @leslieg, @jquijano, @mshaik, and @phenaproxima.
Adam and I suggested that we move AU to require from require-dev, so that it is always in the codebase. Then the message can link to the module install page. But it retains the ability for people to NOT use Package Manager.
tim.plunkett → made their first commit to this issue’s fork.
There's a test for pagination across multiple project browser instances that *should* be able to test this too.
Look for \Drupal\Tests\project_browser\FunctionalJavascript\MultipleInstancesTest::testIndependentPagination()
Thanks for addressing the feedback @kunal.sachdev
Fixed
Not to put too fine a point on it, but let's leave this open until the beta is tagged.
Merged !779 🎉
Saving credit
With the move to Attributes, I think this is safe to close
tim.plunkett → created an issue.
tim.plunkett → created an issue.
Also those ignores were done conditionally: 📌 Switch to conditional usage of PHPStan for upstream incpompatibilies Active
What job is this on? hard to tell from the link above.
I am getting PHPstan passes locally with both PHPStan 1 and 2
Sorry @juandhr, it was easier to just fix directly.
@param array<string|int>|null $ids is what the other MR does, and that's the more PHPStan-like syntax, and also is more descriptive than just mixed[]
Confirmed.
This can be closed.
But a bit of history:
That constant was removed from core in March 2014.
That hook never existed as proposed, it was instead committed as hook_menu_link_defaults() and was then promptly removed in July 2014 in favor of the current *.links.menu.yml approach.
Merged !780 🎉
Saving credit
tim.plunkett → made their first commit to this issue’s fork.
Merged !778 🎉
Saving credit
Merged !762 🎉
Saving credit
Merged !766 🎉
Saving credit
Merged !769 🎉
Saving credit
Merged !774 🎉
Saving credit. Excellent work!
Merged !776 🎉
Saving credit
#15: https://git.drupalcode.org/project/drupal_cms/-/blob/1.x/project_templat...
/**
* Implements hook_form_alter() for install_configure_form.
*/
function drupal_cms_installer_form_install_configure_form_alter(array &$form): void {
// We always install Automatic Updates, so we don't need to expose the update
// notification settings.
$form['update_notifications']['#access'] = FALSE;
}
Merged !770 🎉
Merged !764 🎉
Saving credit
Merged !765 🎉
Saving credit
Saving credit
This is so clean, love to see it.
Merged !773 🎉
tim.plunkett → made their first commit to this issue’s fork.
PHPStan is never wrong™!
Love the refactor since yesterday, as well as the docs on the render element. Great work!
Merged !768 🎉
Saving credit
Merged !767 🎉
Saving credit
Merged !733 🎉
Saving credit
Merged !751 🎉
This looks great. The changes to main.js are so clean!
I don't feel that strongly, will let Chris decide.