Problem/Motivation

It would be great to deprecate auto including .inc files defined by hook-hook-info groups.

In order to do that we first need remove helpers from provided groups.

This issue handles all of the functions related to views_field_default_views_data.

We replace the functionality, then deprecate the functions and move them to the .module files.

We use DI for the replacements only on a couple since views related dependencies need to be optional so they must be last.

The hooks using these services should be converted to DI in a followup.

Steps to reproduce

N/A

Proposed resolution

Move it to a service in the views module.
Replace calls.

Remaining tasks

Open follow up to add dependency injection to the hook classes using this. I didn't want to add this as the first injected dependency, but also didn't want to add DI to all the hook classes using this as part of this issue.

User interface changes

N/A

Introduced terminology

N/A

API changes

Deprecated functions:
views_field_default_views_data
datetime_type_field_views_data_helper
_views_field_get_entity_type_storage
_content_moderation_views_data_object

See cr for replacements.

Data model changes

N/A

Release notes snippet

Issue fork drupal-3489415

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Related issues: +#3489414: Deprecate datetime_type_field_views_data_helper
nicxvan’s picture

nicxvan commented
Issue summary: View changes

nicxvan opened merge request !10318

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Status: Active » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

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

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

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 #2337515: Allow @FieldType to customize views data.

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?

nicxvan’s picture

nicxvan commented

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 short term too, but this at least makes it more reusable and deprecated it.

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work
nicxvan’s picture

nicxvan commented
Title: Deprecate views_field_default_views_data » Deprecate views_field_default_views_data and related functions
nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work
nicxvan’s picture

nicxvan commented

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.

I asked @catch for advice, I'm going to create another issue to test @berdir's suggestion.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

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.

nicxvan’s picture

nicxvan commented

Let me test making it optional.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
nicxvan’s picture

nicxvan commented

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

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

Gotta handle content_moderation.views.inc as well.

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

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review

I added content moderation.

@longwave confirmed I should keep them separate.

nicxvan’s picture

nicxvan commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented
Related issues: +#3069442: Deprecate views_entity_field_label() in favor of EntityFieldManager->getFieldLabels

nicxvan opened merge request !10430

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

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.

nicxvan’s picture

nicxvan commented

Rebase looks good.

Comments are resolved, just waiting on tests.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
nicxvan’s picture

nicxvan commented

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.

nicxvan’s picture

nicxvan commented
Issue summary: View changes

smustgrave closed merge request !10318

nicxvan’s picture

nicxvan commented
Issue tags: +11.2.0 release priority
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Appears to need a rebase.

nicxvan’s picture

nicxvan commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented

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

nicxvan’s picture

nicxvan commented

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

nicxvan’s picture

nicxvan commented

Added #3496775: [policy, no patch] Clarify deprecation policy 3.2 to follow up on the test discussion.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

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

nicxvan’s picture

nicxvan commented

Addressed most comments, asked for clarification on another.

nicxvan’s picture

nicxvan commented

I addressed your last comment and renamed it getSqlStorageForField.

I updated the cr too.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

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.

nicxvan’s picture

nicxvan commented

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

  • catch committed 1da8aa09 on 11.x 
    Issue #3489415 by nicxvan, berdir, smustgrave: Deprecate...

catch closed merge request !10430

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

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!

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Related issues: +#3502649: Resolve PHPStan Errors and PHPUnit 10 Compatibility Issues in Drupal 11+

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.

catch’s picture

catch
he/him
Primary language English
commented

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

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

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.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

nicxvan’s picture

nicxvan commented