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

Looks like it's going to be problematic replacing the code in method \Drupal\file\Plugin\Field\FieldType\FileItem::doGetUploadLocation() with a trait because it is static.

For the new trait could we get a CR

Fixed merge conflicts and added a change record.

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

For the rebase.

Merged with 11.x

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.

• Rebased on 11.x
• Renamed to getUploadLocation() as per the suggestion in #9
• Updated the IS and CR

I really wanted to replace \Drupal\file\Plugin\Field\FieldType\FileItem::getUploadLocation() with the trait but not sure how to work around deprecating a static method with a class method.

thoughts on handling that replacement in a follow up?

Created follow-up 📌 [PP-1] Replace FileItem::getUploadLocation() with FileUploadLocationTrait Active

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

I thought I would have a go at replacing FileItem::doGetUploadLocation() with the trait method.

Unfortunately, generateSampleValue() calls it and is a static method, so we have to call the trait method statically too. I added a @phpstan-ignore-next-line but I'm not sure what the alternative is?

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.

Updated IS

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.

#23 @alexpott I'd prefer to use a service, as we depend on 'token' and that has about 5 other dependencies.

I've changed \Drupal\file\Upload\UploadDestinationResolver::getUploadDestination() from accepting array $settings to FieldDefinitionInterface and I think that works better.

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.

#26 hmm, not sure exactly how that would work 🤔

@alexpott re: #26 I added the method to\Drupal\file\Plugin\Field\FieldType\FileFieldItemList::getUploadLocation() but not sure how to access that from a $field_definition.

@alexpott thanks for the review, but we still need to work out how to do this. See #28

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.

I took the approach suggested in #30 of creating a file item from the field definition. I put it in a trait so it could get used by both REST and JSON API.

@alexpott Do we still want to postpone the deprecation to 12.x? It seems much less disruptive now.

Updated title and IS

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!

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

kim.pepper → closed merge request !4876