Deprecate the function _views_file_status()

adding tags.

I'm taking this one on :)

I did a search on where _views_file_status() was used and found it's only used in 2 places: in Drupal\file\Plugin\views\filter\Status getValueOptions() and in Drupal\file\Plugin\views\field\Status render().
Both used the function in a different way and expected different results from the function: the filter wants a value options map of possible status values and their corresponding labels, to display in the UI; the field wants to display the appropriate label for the given status value, or a default label if the value can't be matched.

I've inlined this functionality on both places and simplified/clarified the code implementation a little bit.

Inlining the code obviously solves any (auto)loading problems so if nobody is opposed to it, I think this fixes the original problem?

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/Status.php
@@ -21,7 +21,16 @@ class Status extends FieldPluginBase {
+  private function getFileStatusLabel($value) {

We never use private in Drupal, this should be protected.

For extra credit you could also add a "public" on the function that is in the patch context above. Not strictly part of this, though.

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/Status.phpundefined
@@ -21,7 +21,16 @@ class Status extends FieldPluginBase {
+  private function getFileStatusLabel($value) {
...
+    return isset($value) && isset($valueToLabelMap[$value]) ? $valueToLabelMap[$value] : t('Unknown');

Here $value should be always set, as it is not optional

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/Status.phpundefined
@@ -21,7 +21,16 @@ class Status extends FieldPluginBase {
+  private function getFileStatusLabel($value) {

This should be protected, we generally don't make any methods private.

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/field/Status.phpundefined
@@ -21,7 +21,16 @@ class Status extends FieldPluginBase {
+      FILE_STATUS_PERMANENT => t('Permanent'),

We need to make this a class constant instead, then we can unit test this stuff easier.

+++ b/core/modules/file/lib/Drupal/file/Plugin/views/filter/Status.phpundefined
@@ -21,8 +21,10 @@ class Status extends InOperator {
+        0 => t('Temporary'),
+        FILE_STATUS_PERMANENT => t('Permanent'),

I wonder if there is somewhere central we can get this from? If we could remove the duplication that would be great.

Sorry, xposted! This issue has got busy all of a sudden ;)

I've incorporated the feedback from #4, #5 and #6 and went a little further ahead to allow for more reuse: I created a very small (centralised) FileStatusMap utility class that has the functionality I inlined in the previous patch (and before that was _views_file_status()).

I've also added a UnitTestCase implementation for testing this little new class.

The trick for using/re-defining the t() function in the test comes from \Drupal\views_ui\Tests\ViewListControllerTest.

I was unsure about the FILE_STATUS_PERMANENT const from core/includes/file.inc, I opted to mirror it inside the new class (FileStatusMap) for testability/encapsulation since those include files don't really exist in the OO space anywhere.
But introducing duplicate code like this is also not ideal so ... what do you think?

:( I accidentally uploaded that interdiff with a .patch instead of a .txt extension ... I can't edit it anymore ... if it makes the original patch fail I'll just repost everything correctly.

If you replace something with a constant somewhere else I think all the usages should be replaced as well. Maybe skip this in that patch for now and open a new issue for just that.

I fixed the missing constant reference that made the test fail (that was referencing the original constant in core/modules/file.inc, which still exists, I just mirrored it as a class const so that class is standalone and could be tested in isolation)

I've added interdiffs as well so you can see this last change just changes the constant.

I am sorry but I think we should move the existing usages and not just introduce a new one.

Update status :(

Hi,

so what is actually happening with this issue? As far as I understand the reason it still needs work is that use of the constant? I have reviewed the code and it does not seem to have any flaws so if the problem is the constant I can rewrite that part. Otherwise - could you please let me know what exactly is to be done? I`d like to help with that.

Unassigning @Thomas Brekelmans... Hop back on if you want.

The current only usage of _views_file_status() is in Drupal\file\Plugin\views\filter\Status::getValueOptions().

Unwrap some of the logic and inline it with that method, and here's the patch.

Thanks @Mile23!

Based on the Drupal 8 allowed changes policy we should now generally deprecate stale procedural functions in Drupal 8 rather than removing them. In this particular case, since its name begins with an underscore, that's implicitly considered an "internal" API, so we could consider removing it in a minor. We'd need to clarify that on the BC policy though since it does not mention the underscore naming pattern.

This probably needs release manager input before proceeding in one of those directions, though, so it's not currently a good novice task per https://www.drupal.org/core-mentoring/novice-tasks. Removing the novice tag.

Resurrecting this after spotting it following #3021833: Move FILE_STATUS_PERMANENT to \Drupal\file\FileInterface

It all looks good to me. I have just 1 nitpick question. After that is it RTBC for me.

+++ b/core/modules/file/src/Plugin/views/filter/Status.php
@@ -15,7 +16,10 @@ class Status extends InOperator {
+        0 => $this->t('Temporary'),

Do we really need to add 0 => or can it be removed? I can see that _views_file_status() did it. It is something we normally do not do in core.

Added a change record: https://www.drupal.org/node/3227228

Re #33 I think it is better to be explicit here that 0 means temporary, we usually only omit the array keys when we don't care about the number, but here the 0 has meaning.

Updated the Is and the CR.

The docblock of the function _views_file_status() is missing the text: "@deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. There is no replacement."
If you are a novice do a code base search for the tag "@deprecated in drupal:9.3.0" to see how it should be done.

For got the needs work.

Updated the patch #34 based on the comment in #36. Please review.

Thanks, the coding standards bot found a little issue:

 1781 | ERROR | [x] Separate the @return and @deprecated sections by
      |       |     a blank line.
 1781 | ERROR | [x] Whitespace found at end of line

Updated the patch #39 to fix the coding standard. Hope this works.!!

It looks good to me.
For me it is RTBC.

Committed/pushed to 9.3.x, thanks!

Push didn't make it - I've done that an also published the CR.