πŸ‡ΊπŸ‡ΈUnited States @tim.plunkett

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

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡Έ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.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

With the move to Attributes, I think this is safe to close

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

tim.plunkett β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

What job is this on? hard to tell from the link above.

I am getting PHPstan passes locally with both PHPStan 1 and 2

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Sorry @juandhr, it was easier to just fix directly.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

@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[]

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Confirmed.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Merged !780 πŸŽ‰

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Merged !778 πŸŽ‰

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Merged !766 πŸŽ‰

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Saving credit

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Merged !774 πŸŽ‰

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Saving credit. Excellent work!

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

#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;
}
πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Merged !770 πŸŽ‰

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Merged !764 πŸŽ‰

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Merged !765 πŸŽ‰

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Saving credit

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

This is so clean, love to see it.

Merged !773 πŸŽ‰

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

PHPStan is never wrongβ„’!
Love the refactor since yesterday, as well as the docs on the render element. Great work!

Merged !768 πŸŽ‰

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Merged !733 πŸŽ‰

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Saving credit

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Merged !751 πŸŽ‰

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

This looks great. The changes to main.js are so clean!

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

I don't feel that strongly, will let Chris decide.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Re-reviewed, re-tested, all looks good. Awesome work @utkarsh_33!

πŸ‡ΊπŸ‡Έ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

#4 makes me think we could add tests for this, right? Some extra fixtures for testRecipesAreDiscovered

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

I think Chris should review this one as he was able to reproduce the flickering.
But this looks good and completely explains why there was weirdness.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

I left the self-review comments from @phenaproxima in-place for Chris, but all my concerns are addressed.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Ugh, we can't fully do this until we drop support for 11.1.x, which is a long ways off (or never?)
That sucks.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Reverted my commit, will address in follow-up. This is good to go

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

My main point was going to be about a bit of minor code duplication between the RandomData plugin and the Mock plugin, which led to me asking why we still have both, which is answered by πŸ“Œ Remove the RandomData source Active . So this is good for now, and we'll clean up that duplication there!

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Reviewed this, and even went down a rabbit hole of the safety of hash_file(), but we're fine because of the file_exists() check (and because the only other error condition is if the file being hashed is over 2GB, and if your composer.lock is over 2GB, you are beyond help)

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

If this is still desired for PB, it needs to be restarted for the 2.x branch

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

As discussed, #107 is half pre-existing, half-out-of-scope.
#109 is in response to that, but unchanged here.

I updated the CR per discussion with @lauriii.

I rebased the branch after fully re-reviewing it again.

Thanks @kunal.sachdev!

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Awesome, thanks @fjgarlin for debugging!

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Rebuilt this based on how XB is handling it, with @todos pointing to πŸ“Œ Only run linting jobs if the files changed make sense for the job Active

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

tim.plunkett β†’ changed the visibility of the branch 2.0.x to hidden.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

#21 all three of those methods are being removed from DrupalDotOrgJsonApi and as of this MR now only exist on DrupalDotOrgSourceBase.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Great! This is a nice modernization.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

tim.plunkett β†’ changed the visibility of the branch 3322601-pp-1-add-instructions to active.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

tim.plunkett β†’ changed the visibility of the branch 3322601-pp-1-add-instructions to hidden.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Merged !726 πŸŽ‰

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

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

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Even if the other issue about stripping out inheritdoc landed, I think this issue still has merit. There is value in the knowing the intention of an overridden method, and this isn't a Drupal-ism or from Symfony or Laravel, it's a language-level "feature".

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

This is a great clean-up. I'd like to see a test for #17, I'm actually surprised PHPStan didn't catch that. But that can happen in a follow-up.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

To be clear, this is an MR I made of the branch @joachim of the patch @claudiu.cristea made in 2018. There's definitely a lot of cruft in it.

@nicxvan feel free to push to the MR fork!

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Thanks @nod_! Rebased

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

This is fine, it undoes a reasonable portion of what πŸ› Remove certain presentational opinions Active overdid

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

I've reviewed this and the new upstream code, great work.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

tim.plunkett β†’ changed the visibility of the branch 3386762-open-field-settings to hidden.

πŸ‡ΊπŸ‡ΈUnited States tim.plunkett Philadelphia

Sorry, I forgot this included the fix from πŸ› Compressed ajax_page_state['libraries'] can exist in both $request->request and $request->query simultaneously Active , so this is technically postponed. There's also a failing test now, and it might have been from the last commit.

#99 Can you file a follow-up to fix that for Safari?

Production build 0.71.5 2024