🇺🇸United States @tim.plunkett

Awesome!

Reviewed!

Awesome, thanks @fjgarlin for debugging!

lol

Split off my work to 📌 Switch to conditional usage of PHPStan for upstream incpompatibilies Active and restored the branch

tim.plunkett → created an issue.

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

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

tim.plunkett → changed the visibility of the branch 2.0.x to hidden.

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

Great! This is a nice modernization.

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

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

Merged !726 🎉

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

All feedback addressed.

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

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.

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!

Thanks @nod_! Rebased

This is fine, it undoes a reasonable portion of what 🐛 Remove certain presentational opinions Active overdid

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

phenaproxima → credited tim.plunkett → .

tim.plunkett → changed the visibility of the branch 3386762-open-field-settings to hidden.

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?

I cleaned up the new CR and deleted the outdated CR.

Good news: returning [] and NULL have exactly the same effect. We can file a follow-up to fix BlockViewBuilder which has been wrong this whole time. But let's not let it slow us here.

Converted the patch to an MR and added more tests.

Switched to an MR

Switched to MR

Switched to MR

Switch to MR

Turned patch into MR.

Agreed

hestenet → credited tim.plunkett → .

Merged !674 🎉

Saving credit

Merged !692 🎉

Saving credit

Merged !667 🎉

Thanks for your input @poker10. Drupal CMS and Drupal Core are different products.

Saving credit

Merged !693 🎉

Thanks! Saving credit

surabhi-gokte → credited tim.plunkett → .

Thanks! Good clean-up

This is such a nice clean-up. Farewell stores.js!

N.B. this is mainly a problem because code like HtmlResponseAttachmentsProcessor, CssCollectionOptimizerLazy, and AjaxBasePageNegotiator all use code like

  $this->requestStack->getCurrentRequest()->get('ajax_page_state')

and Request::get() checks attributes, query, and request in that order.

tim.plunkett → created an issue.

thejimbirch → credited tim.plunkett → .

MR looks good, and we have buy-in on the idea. Thanks @phenaproxima and @catch

Important to note: the merge Lauri did failed tests, my commits after are similarly failing. We might want to revert my commit of the dialog.js changes until we get the tests green again

Merged !634 🎉

fjgarlin → credited tim.plunkett → .

Saving credit

This was merged, which is good, but by me, which is not.

Sold.

Opened 🐛 Resolve type issues caught by PHPStan usage in Project Browser Active for the actual "bugs" upstream in Package Manager (as opposed to the ones stemming from the weirdness of legacy core code).

tim.plunkett → created an issue. See original summary → .

One last step is to document the additions to the phpstan.neon

tim.plunkett → created an issue.

Interestingly enough, I was messing around with higher levels of PHPStan and Level 4 would have caught at least the part from 🐛 $source !== 'recipes' is a comparison of incompatible types Active

Re #35/#36, adding this to core/phpstan.neon.dist doesn't help contrib modules (since extending core's is a bad practice), but adding it to your own does help.

  universalObjectCratesClasses:
    - Drupal\Core\Extension\Extension

+1 to RTBC for all of the tweaks since my initial fix. That installer decorator change being the needed fix is wild. I was half-wrong in #7, not obvious at all!

quietone → credited tim.plunkett → .

Yes, that is an alternate approach to fixing the same bug.

For some reason, this MR causes a regression on 10.4.x

For example, running \Drupal\Tests\project_browser\Functional\InstallerControllerTest::testCoreModuleActivate() now gets this error on activation:

{"message":"PluginNotFoundException: The \u0022filter_format\u0022 entity type does not exist.","phase":"project install"}

I'm sure the reason will be obvious once it's tracked down, but wow that's a weird bug right now.

Only one manager remains in a Type directory: \Drupal\rest\Plugin\Type\ResourcePluginManager
That class really shouldn't be in there, but unlike all the situations described in the original issue summary, there are no other classes in that directory.
It *was* confusing when the manager was among the other classes, but that's not happening here.

I think if anyone feels strongly now, they can open an issue to move ResourcePluginManager to a different directory, but this can be closed (and not just because it's stale).

100% correct

tim.plunkett → created an issue.

tim.plunkett → created an issue.

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

I think this is a duplicate of 🐛 WSOD when using settings to limit sources to recipes Active
Or at least they are different approaches to the same underlying problem

Duplicate of 🐛 Symfony\Component\Routing\Exception\MissingMandatoryParametersException: Some mandatory parameters are missing ("source") to generate a URL for route "project_browser.browse". in Drupal\Core\Routing\UrlGenerator->doGenerate() (line 180 of core/lib/Drupal/ Active (even though this was opened first!)

Admin Toolbar Extra Tools adds a menu link for Project Browser:
https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...

However, that URL won't work anymore now that there's no default source and the source is required.

There's a chance this could be mitigated by 🐛 WSOD when using settings to limit sources to recipes Active .
Both are tied to the changes made in ✨ Make all enabled sources exposed as local tasks Active .

It's not clear if the report in #4 is related, that seems to be about Oracle DBs?

To be clear, it's not a trial-only problem right now. If someone were to stumble upon /admin/config/development/project_browser and disable the "Contrib modules" source, this task would still exist and when clicked would trigger a WSOD.

Leaving this postponed in case the direction of 🐛 WSOD when using settings to limit sources to recipes Active shifts, but as the MR exists right now it will resolve this problem.
It turns that WSOD into a 404, and since the Top Tasks are menu links, it's smart enough to hide the task on it's own.

tim.plunkett → created an issue.

Working on this. It's even easier to reproduce than the IS, just go to /admin/modules/browse/asdf. It doesn't just matter if it's enabled/disabled, it's also whether it exists at all.

Targeted MR already exists here, is more needed?

@soaratul and I synced up on this just now.
In standard Drupal routing, having a slash or not having a slash is no issue, and it's just preference (for context, Drupal Core precedes all of its routes with the slash).

However, when you use Symfony's autowiring, it has to match 1:1 with the definition in the services.yml file.
And those definitions must NOT contain a slash!

As I mentioned before, core's routes are prefixed with a slash, EXCEPT for the ones that are autowired (e.g. WorkspacesHtmlEntityFormController).

Additionally, all of the existing routes in XB are all correctly not using a prefixed slash.

If you add a slash to any of the existing XB autowired routes, you will get the same error from ClassResolver.

The slash was added in 📌 Create an endpoint to publish all auto-saved entities Active
The only reason that issue worked is because the test coverage instantiated ApiPublishAllController as a service, not using the routing system.

So as soon as ✨ [PP-1] Implement the "Publish All" button Postponed attempted to use it as a route, this error was hit.

My recommendation would be to merge this fix into the MR for the preview issue and close this one.

Needs a rebase but +1 from me

Thanks @phenaproxima!

Barring any future improvements to actually preventing people from being able to even try, I think putting it in the README is good. Thanks!