Deprecate views_field_default_views_data

datetime_type_field_views_data_helper() calls views_field_default_views_data(), why not do those two together?

Additional thoughts:

This is all part of hook_field_views_data(), basically each of those hooks is expected to call into that function, there are also several calls in contrib implementations of that hook and only that hook.

There are several issues open about limitations of that hook, namely that it only works for configurable fields. There's @todo pointing to [ #2489476], which is postponed on 📌 Allow @FieldType to customize views data Needs work .

I'm all for getting rid of hook_hook_info(), but converting these functions to helper services will also conflict with and delay those fairly important issues further and we might change them again as part of that.

What about a quickfix were we just move the functions to views.module? Then we could leave it to those issues to refactor them into the new handlers?

We can combine this and datetime, I was trying to keep them clean but they are coupled.

We could just move them, but that's a short term bandaid and those issues are a decade old and using annotations still.

Though I suppose this would be a bandaid too.

I'm beginning to wonder if ViewsFieldDefaultViewsData should be moved to a core service somewhere, there are a lot of tests failing cause views is not installed.

While this is a helper for views, I don't think this should require views to be installed. If you want to use datetime without views you should be able to.

The issue is when hooks are discovered the service is injected and it can't find it.

Yeah, that would require conditional services or at least conditional constructor arguments. based on a recent comment I saw, you can apparently just make them optional in the definition and Autowire will handle it.

That is an interesting problem though not just for this issue but in general. hook implementations so far have worked well for optional dependencies/integrations, if they are not called, they don't cause any problems. But now combined with DI, it gets a bit more complicated.

Let me test making it optional.

Making optional worked. It's only called when views is installed so it will be available when needed.

Gotta handle content_moderation.views.inc as well.

I think I really need to include the field label issue here too.

I added content moderation.

@longwave confirmed I should keep them separate.

I have to compare the newest MR with the previous version of the older MR and transfer comments over, I messed up the rebase and had to start over.

nicxvan → changed the visibility of the branch 3489415-deprecate-viewsfielddefaultviewsdata to hidden.

Rebase looks good.

Comments are resolved, just waiting on tests.

Moving comment that is still relevant from closed mr to here.

@berdir

I know we've been through this, and yes, the other issues are very old and somewhat stale, the problem is that all this is really pretty much for nothing. It all has to change again. It's a major problem that all these APIs and methods are hardcoded to configurable fields and field module interfaces, we need them to use the FieldStorageDefinitionInterface, not this. It's a lot of work, not just this, but the deprecation and work that contrib has to do, only to deprecate all this again. Lots of conflicting thoughts, if we start to make more and and more changes like change the types I'm not sure if we can stop and progress
I guess I'd still prefer to just move the functions and not do this conversion to services ;)

Me

Yeah, this is a simple deprecation though, you just need to replace the function with the service call.

smustgrave → closed merge request !10318

Appears to need a rebase.

Large change. Believe we should have deprecation tests added to make sure people will be getting the message.

Discussing on slack for clarity, tests for deprecation messages explicitly are against deprecation policy https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... → 3.2

Added 📌 [policy, no patch] Clarify deprecation policy 3.2 Active to follow up on the test discussion.

I reviewed this again. some feedback came up like this already before I think, but it's IMHO still true.

Addressed most comments, asked for clarification on another.

I addressed your last comment and renamed it getSqlStorageForField.

I updated the cr too.

Came here from slack.

So seems this has already been reviewed a few times. I resolved the 1 thread on the MR as it seemed the consensus was getSqlStorageForField().

Left 1 question on the MR but by no stretch I think a blocker or possibly even needed.

I did review the CR since we talked a little about those in slack. Very well organized. Code examples with before/after are always appreciated.

Going to go on a limb and say this one is good to go.

Added some clarification for your comment I'll keep an eye on tests.

catch → closed merge request !10430

This looks good to me. I would generally agree with @berdir that it's better to do one change than two to the same code, especially if it leads to a double deprecation, but given the staleness of the issues this conflicts with, it shouldn't be too much churn.

Committed/pushed to 11.x, thanks!

To maintain backward compatibility for 10.x while it’s still supported, is there a way to replace views_field_default_views_data without relying on the new service views.views_field_default_data that doesn't exist till 11.2? This would help ensure compatibility with existing setups in contrib.

@joelpittet could you use https://www.drupal.org/node/3379306 → ?

The function still exists as a BC layer, it's just moved to to the .module file, so you don't have to do anything now.

You _can_ prepare already for D12 by either using the DeprecationHelper or a \Drupal::hasService() check with fallback to the function, but that's entirely optional for now.

Thank you both Claudiu did the DeprecationHelper which works for me, I didn't realize that existed.