[pp-1] Deprecate FIELD_LOAD_* constants

The actual usage is internal.

I guess the problem are the places that happen to use it from the outside, like EFQ (should really be a different constant, as it has a different meaning), maybe use loadRevisions() instead of age(constant)?

Then there's views integration, no idea what that is exactly doing and that file reference stuff...

Marked #2029709: Convert FIELD_LOAD_CURRENT to a constant on some class/interface as a dupe of this.

rephrase

Is this issue valid? Constants are in EntityStorageControllerInterface
Suppose it make sense to remove FIELD_ prefix

4: 2081513-4.patch queued for re-testing.

4: 2081513-4.patch queued for re-testing.

Hello
Replaced FIELD_LOAD_* constants by boolean flag.
Removed QueryInterface::age() and created a new method QueryInterface::loadRevisions().

Regards

In field.views.inc I found the use of FALSE/TRUE as array keys entirely unintuitive, and this also seems to be the cause of the test failure. Instead it seems safe to use strings here, as they are only ever referred to inside the same function; the attached patch uses 'current' and 'revision' here.

Discussed this issue with @Berdir in IRC as it is related to #2458543: Entity query age(EntityStorageInterface::FIELD_LOAD_REVISION) only gets current revision ID. That issue makes this one much simpler and also makes the constant have even less meaning and relevance. So this is a followup of that issue.

Also the use of the $age param in file_get_file_references() looks very broken.

Yes, only usages in file and views are remaining I think.

Anyone up for reroll?

Tagging as novice for the reroll.

Patch rerolled

That reroll happened a bit early, #2458543: Entity query age(EntityStorageInterface::FIELD_LOAD_REVISION) only gets current revision ID was not yet committed. Let's try again :)

I think better to use $load_revisions as param name, because it mans to load all revisions on not

+++ b/core/modules/file/file.module
@@ -1431,13 +1427,13 @@ function file_icon_map($mime_type) {
-function file_get_file_references(FileInterface $file, FieldDefinitionInterface $field = NULL, $age = EntityStorageInterface::FIELD_LOAD_REVISION, $field_type = 'file') {
+function file_get_file_references(FileInterface $file, FieldDefinitionInterface $field = NULL, $load_revisions = TRUE, $field_type = 'file') {
...
-  if (!isset($references[$file->id()][$age])) {
-    $references[$file->id()][$age] = array();
+  if (!isset($references[$file->id()][$load_revisions])) {
+    $references[$file->id()][$load_revisions] = array();

@@ -1470,7 +1466,7 @@ function file_get_file_references(FileInterface $file, FieldDefinitionInterface
-                $references[$file->id()][$age][$field_name][$entity_type_id][$entity->id()] = $entity;
+                $references[$file->id()][$load_revisions][$field_name][$entity_type_id][$entity->id()] = $entity;

@@ -1479,7 +1475,7 @@ function file_get_file_references(FileInterface $file, FieldDefinitionInterface
-  $return = $references[$file->id()][$age];
+  $return = $references[$file->id()][$load_revisions];

+++ b/core/modules/file/src/FileAccessControlHandler.php
@@ -62,7 +62,7 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
-    return file_get_file_references($file, NULL, EntityStorageInterface::FIELD_LOAD_REVISION, NULL);
+    return file_get_file_references($file, NULL, TRUE, NULL);

This is just swapping the param name - fine but it actually does not do anything. Which is a bug. And looking at FileAccessControlHandler - is this a security issue?

This issue is a major task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

@Berdir does the issue still makes sense for 8.0?

8.1 material, but needs BC now

Chaning title to reflect new aim

Wondering what we want to do exactly? There are two remaining uses for that constant:

1. file_get_file_references() very strange left-over that uses that constant in a completely unrelated way. I guess we could add new constants with the same value, but I'd rather just move the whole function to a service with a better API and deprecate the old function completely.

2. (apparently) completely internal usage in views_field_default_views_data(). We could just stop using them there I think and instead use a string or so. Or we could just keep them and deprecate anyway.

1. would need a separate issue I think and this would be blocked on that, after that we could just deprecate the constants and either update 2. to use something else or don't care and assume the whole function will sooner or later be rewritten or moved and deprecated as a whole (e.g. there's the pending issue of integrating per field type views data into EntityViewsData).

That said, whatever we do, I don't think this is a major anymore. Usage of this was far more widespread when this issue was created and I think the most problematic/confusing one was in entity queries. But any usage in storage handlers and entity query has been refactored away in other issues.

And one remaining public usage in an API function that is weird on its own isn't a major problem IMHO.

@Berdir now revision api in core any idea how to clean-up this?

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

#30 still stands today, the weirdness is that the constants are defined in core then used once in file.module and once in Views, but these uses could probably be swapped to self-contained constants in each of those cases; it doesn't make much sense to share them. Neither of the things in #30 has happened in the time since that was posted, though. Maybe we just make those issues, postpone this on them happening, and wait another ten years?

Can they be enums?

> Maybe we just make those issues, postpone this on them happening, and wait another ten years?

the file module usage is deprecated in #1452100: Private file download returns access denied, when file attached to revision other than current.

views is just a purely internal array key, enum isn't a good fit for that, I'd just duplicate the constants into that class and then deprece them here. Could be done in the same issue too once file.module is done.

Which happens to be exactly what I wrote in 2015, reading up, sadly the rewriting part of that code didn't happen beside moving it into a service.