Add a trait to get file upload location from a field definition

For the new trait could we get a CR

@kim.pepper Need to rebase the branch.
Thanks!

For the rebase.

The trait seem to be functioning well for the file fields. I was considering whether it might be sensible to maintain consistency in the function names within the trait as well. For instance, changing getFileUploadLocation to getUploadLocation(). Just a suggestion.

Thanks for fixing that @kim.pepper!

I'm triaging RTBC issues. I read the IS, the comments, the change record and the MR. I do like a bit of tidying up!

The question raised in #9 has not been answered. Why the name change? And how will this effect any contrib usages of getUploadLocation?

The change record also needs to be updated, it is referring to changes that have not been made. Also, the first sentence is difficult to read, and English is my first language. Also, instead of mentioning a usage in a contrib project it would be more helpful to explain what must be done in contrib to update modules for this change, such as a before and after example.

Setting to needs work for the above.

thoughts on handling that replacement in a follow up?

Oh cool. Then in that case rest seems fine to me.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Reviewed the deprecation and link to CR appears correct https://www.drupal.org/node/3406099

Reviewing actual CR and all the details are there.

Believe this is good.

This is my second visit here for triage. I didn't find any unanswered questions or other work to do.

Leaving at RTBC;

I wonder if we don't have to add the service. Can we use the same approach as \Drupal\media_library\Form\FileUploadForm::createMediaFromValue and create a FileItem to do this.

Otherwise if we're going to go with the current approach I think we should be accepting a field definition object instead of settings and validating that it is a fiie item that we're generating a location for. Otherwise it's all a bit mushing random arrays together.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I've looked at both this and \Drupal\file\Validation\FileValidatorSettingsTrait::getFileUploadValidators() and I think that they both should be / have been part of the file field API. If we put them on \Drupal\file\Plugin\Field\FieldType\FileFieldItemList then we'd be able to call this methods from the field definition we already have.

I don't think our API should be decided by the fact that our field system struggles with container injection (only because it was updated before the container was truly available in the D8 cycle). Both getFileUploadValidators() and getUploadDestination() are extremely intimate to the internals of the file field type.

Yeah having this on the field item list doesn't work out :( I can't see how to get back to the list item either...

$field_definition->getClass() returns \Drupal\file\Plugin\Field\FieldType\FileFieldItemList but $field_defintion is actually a FieldItemDataDefinition.

How about not deprecating the stuff on the FileItem and use that - as suggested in #23.

Given they are both controllers I think 11.x is fine

Appears feedback has been addressed.

CR still appears fine, not sure if it was updated or mention removed in 12 before.

Committed and pushed 7a12710863 to 11.x and e9c7c01bc8 to 10.3.x. Thanks!

I've encountered a significant issue with the implementation of this change that affects the deprecation mechanism and backward compatibility.

The problem is that the current implementation uses a trait alias in FileUploadResource:

use FileUploadLocationTrait {
    getUploadLocation as getUploadDestination;
}

This creates several problems:

1. Deprecation warnings are bypassed: When a class extends FileUploadResource and overrides `getUploadLocation()`, the overridden method is never called because the core code now calls `getUploadDestination()` instead. This means the deprecation warning in the original `getUploadLocation()` method is never triggered.

2. Silent breakage: Existing code that extends FileUploadResource and overrides `getUploadLocation()` silently stops working without any indication to developers that their code needs updating.

3. Counter-intuitive override mechanism: Currently, if developers want to override the upload location logic, they must create a `getUploadDestination()` method instead of overriding `getUploadLocation()`, which is very counter-intuitive.

4. Misleading documentation: The deprecation message says "Use \Drupal\file\Upload\FileUploadLocationTrait::getUploadLocation() instead" but in practice, developers need to implement `getUploadDestination()`.

I've created a separate issue to address this: https://www.drupal.org/project/drupal/issues/3540037

This implementation effectively breaks the standard PHP inheritance model for the deprecation period, which could cause significant confusion and silent failures in contrib/custom modules that extend FileUploadResource.