🇺🇸United States @tim.plunkett

Philadelphia
Account created on 14 February 2008, over 17 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States tim.plunkett Philadelphia

Marking RTBC per discussion with @mglaman

🇺🇸United States tim.plunkett Philadelphia

This was originally proposed before Layout Builder even existed.
Now we will have Experience Builder as well.

I don't know that there's much remaining value in this issue as written, I would move to close it.

🇺🇸United States tim.plunkett Philadelphia

#2.1 That's not the point, RenderElementBase is NOT internal, only the property has_garbage_value is, but because of how the PHPDocs were written it is being parsed as the whole class being internal.

#2.2 Project Browser runs on Level 8, and also provides a Render Element, and will start failing on this.

🇺🇸United States tim.plunkett Philadelphia

@oily except that @acbramley's advice here is appreciated and welcomed, and correct.

🇺🇸United States tim.plunkett Philadelphia

That's actually backwards, the two syntaxes are service:method and class::method. However, the code in CallableResolver has identical code paths after exploding on either amount of :, it processes them the same by passing onto ClassResolver::getInstanceFromDefinition()

Either way, it will work.

🇺🇸United States tim.plunkett Philadelphia

More triage

🇺🇸United States tim.plunkett Philadelphia

Nice work! Verified that the `initial preprocess` approach is the standard practice

🇺🇸United States tim.plunkett Philadelphia

tim.plunkett made their first commit to this issue’s fork.

🇺🇸United States tim.plunkett Philadelphia

tim.plunkett made their first commit to this issue’s fork.

🇺🇸United States tim.plunkett Philadelphia

This looks great, thanks @justafish!

🇺🇸United States tim.plunkett Philadelphia

I think this is changing the directory in which CSpell is run, it's now picking up playwright-report/index.html

🇺🇸United States tim.plunkett Philadelphia

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`

🇺🇸United States tim.plunkett Philadelphia

Basic test coverage was added in a recent commit, see here

🇺🇸United States tim.plunkett Philadelphia

Unable to mark my thread as resolved, but it is. Thanks @phenaproxima!

🇺🇸United States tim.plunkett Philadelphia

#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.

🇺🇸United States tim.plunkett Philadelphia

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.

🇺🇸United States tim.plunkett Philadelphia

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.

🇺🇸United States tim.plunkett Philadelphia

@justafish rightly points out that the fact that this EVER works is concerning, this should probably have some checks around it

🇺🇸United States tim.plunkett Philadelphia

Merged !813 🎉
Thanks

🇺🇸United States tim.plunkett Philadelphia

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.

🇺🇸United States tim.plunkett Philadelphia

tim.plunkett made their first commit to this issue’s fork.

🇺🇸United States tim.plunkett Philadelphia

This change makes sense and the test coverage is clear (and fails without the fix)

🇺🇸United States tim.plunkett Philadelphia

@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 )

🇺🇸United States tim.plunkett Philadelphia

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

🇺🇸United States tim.plunkett Philadelphia

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.

🇺🇸United States tim.plunkett Philadelphia

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

🇺🇸United States tim.plunkett Philadelphia

This should include test coverage for `container`

🇺🇸United States tim.plunkett Philadelphia
🇺🇸United States tim.plunkett Philadelphia

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.

🇺🇸United States tim.plunkett Philadelphia

tim.plunkett made their first commit to this issue’s fork.

🇺🇸United States tim.plunkett Philadelphia

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()

🇺🇸United States tim.plunkett Philadelphia

Thanks for addressing the feedback @kunal.sachdev

🇺🇸United States tim.plunkett Philadelphia

Not to put too fine a point on it, but let's leave this open until the beta is tagged.

Production build 0.71.5 2024