Refactor CurrentContext away

/me jumps down the rabbit hole.

We should wait to do important refactors until after we have the increased test coverage in #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only.

That is, of course, unless we can find a way to be certain we don't break anything. Our last big refactor of the query builder introduced some regressions, and I'd like to avoid that this time.

@e0ipso I absolutely agree! I'm going to get started, but we shouldn't merge this until those tests have been integrated :)

Out of curiosity, which regressions are you referring to? I thought we actually solved multiple issues and uncovered that we were improperly ordering sorts.

I don't think I have seen many bugs related to \Drupal\jsonapi\Context\CurrentContext. Before the last big refactor we had many bugs related to the query builder, that's why the work was necessary. Even without the necessary test coverage I was willing to merge it, knowing that some regressions may/would happen. I was willing to pay the price.

In this case I don't see the clear benefit (one will argue benefits, but those will be hard to prove and the discussion will ultimately result in subjective points of view) like I did with the previous refactor. Taking this on will take a precious time to write and to review.

If our goal is to move JSON API forward into core, we should work on prioritized issues, and I don't think there's consensus that this is a priority. There are other issues where we agree that they are a priority that IMO we should tackle first. If in the other hand the contribution is because you want to have fun with this issue, please do continue on it!

I still think that this should be worked on eventually, it just feels pretty low in my priority list.

FWIW, the following was written before your last reply, @e0ipso.

I'm going to be posting incremental patches to make reviewing things easier. But there's no need to review them until the issue is actually marked as needing review.

The attached patch removes CurrentContext from the FieldResolver. This changes the arguments to FieldResolver::resolveExternal(). However, this method is not actually used anywhere in the codebase and the field resolver is marked as @internal.

@e0ipso, I know you have hesitancy about making changes to internal APIs, but I think that this will be unavoidable for this refactor. The original point of the CurrentContext class was to avoid passing around contextual information like the entity type. This made our method call lengths shorter at the expense of explicitness and testability. So, by definition, we're going to need to alter methods to pass additional context around.

In another issue, you expressed a willingness to create an 8.x-2.x branch. If that would make you more comfortable with a refactoring like this one, I'm all for it. It would allow us to be more liberal with refactoring and clean up. However, I'd like to refrain from actually tagging a 2.x release until we've gotten a lot more than this in it. Of course, all of this should be accompanied by more comprehensive tests.

Now, replying directly to #6:

I completely understand where you're coming from, and I'm fine with this being low on your priority list. This is a bit esoteric, but in my mind, it is part of a greater testability/maintainability epic that will contribute to the overall stability of the module and reduce some technical debt. It's also a way for me to familiarize myself with the module again (since it touches on a bit of everything).

The attached removes CurrentContext from EntityResource. This is a perfect example of how this improves testability. CurrentContext wasn't even used in EntityResource anymore, yet it was still in the constructor and thus needed to be created in all of the EntityResource tests.

Attached removes use of CurrentContext from RequestHandler.

This is done in #2941685: Port parameter based resource config from REST module. Any remaining discussion should be moved there.