Add Data Localization Contexts to OrchardCoreConstants

[hishamco]

Fixes #19034

[sebastienros]

Let's talk about our options here, don't do any changes until we agree on something though.

What about using specific constant classes instead of nesting even more ones. So instead of ORchardCoreConstants.DataLocalizationContext.ContentTypes, why not DataLocalizationConstants.ContentTypesContext, or DataLocalizationContexts.ContentTypes, other suggestions?

Trying to make it easier to read and type, it could get out of control otherwise.

Also maybe the fields one could use a function instead of concatenating, like DataLocalizationContext.ContentFields(string fieldName), suggestions again, let's talk before we act.

[hishamco]

I will open a discussion thread then

[gvkries]

I'd say don't put it into a class called constants if it includes method. IMHO a simple DataLocalizationContexts for all contexts, including methods for fields and e.g. permissions, would be sufficient.

[hishamco]

@gvkries, please move your comment to the issue #19034 then we can take an action after that I can react to the changes

[github-actions]

This pull request has merge conflicts. Please resolve those before requesting a review.

[github-actions]

This pull request has merge conflicts. Please resolve those before requesting a review.

[github-actions]

This pull request has merge conflicts. Please resolve those before requesting a review.

[github-actions]

This pull request has merge conflicts. Please resolve those before requesting a review.

[sebastienros]

I think we should have ContentType(s) in the a library for Content Types (Core, Abstractions, or the module itself if only used locally). Same for content fields or admin menu. Otherwise we'll get every single module leaking in the localization abstractions. And not be an abstractions package anymore.

[hishamco]

@gvkries, could you please check the permissions & admin menus changes? If this is what you discussed in the triage meeting, I will complete the PR ASAP; otherwise, let me know if there's anything else I should take into account

[hishamco]

@gvkries, can you check to move on this one

[gvkries]

I think what you've done for the admin menu and permissions is good. Just apply that to content types/fields as well, so we don't have the constants in the localization abstractions, but in the related modules (or their abstractions).

[hishamco]

@gvkries, please review and merge if all goes well

[gvkries]

LGTM in general, but please check my few comments.

[gvkries]

Why is this removed? Is it the same as another provider?

[gvkries]

Maybe do this change in another PR, also the ListsAdminNodeDataLocalizationProvider probably needs to be renamed (the one in LinkAdminNodeDataLocalizationProvider.cs).

[hishamco]

Why is this removed? Is it the same as another provider?

Let me check ..

Maybe do this change in another PR, also the ListsAdminNodeDataLocalizationProvider probably needs to be renamed (the one in LinkAdminNodeDataLocalizationProvider.cs).

Sure

[hishamco]

@gvkries I remember this no long need coz both admin menus and menu items are using the same provider. This was mentioned a few weeks ago in the steering meeting

[gvkries]

This is required by the ContentFieldLocalizationDataProvider, right? Isn't it better to move the ContentFieldLocalizationDataProvider to the content fields module in that case?

[hishamco]

This is in mind before this PR, but I remember its stay here to avoid module-to-module dependency. I might check this later after this PR

[hishamco]

@gvkries, I added a commit before I noticed your approval. Shall I revert my last commit?

Also, let me know which rename you are refer to in #19022 (comment)

[gvkries]

I think this is fine. For the renaming, look at LinkAdminNodeDataLocalizationProvider.cs.