Problem/Motivation
Remove all uses of file_stream_wrapper_get_* and file_get_stream_wrappers
Proposed resolution
Remove
file_stream_wrapper_get_class($scheme)
with
\Drupal::service('stream_wrapper_manager')->getClass($scheme)
Remove
file_stream_wrapper_get_instance_by_uri($uri)
with
\Drupal::service('stream_wrapper_manager')->getViaUri($uri)
Remove
file_stream_wrapper_get_instance_by_scheme($scheme)
with
\Drupal::service('stream_wrapper_manager')->getViaScheme($scheme)
Remove
file_get_stream_wrappers($filter = StreamWrapperInterface::ALL)
with
\Drupal::service('stream_wrapper_manager')->getWrappers($filter)
Remaining tasks
Provide patch
User interface changes
API changes
Remove all the uses of following function:
file_stream_wrapper_get_class
file_stream_wrapper_get_instance_by_uri
file_stream_wrapper_get_instance_by_scheme
file_get_stream_wrappers
Beta phase evaluation
Issue priority | Normal as the patch removes usage of already deprecated functions |
---|---|
Prioritized changes | code already marked for removal by 8.0.0 |
Comment | File | Size | Author |
---|---|---|---|
#69 | remove_all_uses_of-2392559-69.patch | 29.63 KB | mondrake |
#69 | interdiff_66-69.txt | 822 bytes | mondrake |
#66 | remove_all_uses_of-2392559-66.patch | 29.18 KB | mondrake |
#66 | interdiff_65-66.txt | 1.41 KB | mondrake |
#65 | remove_all_uses_of-2392559-65-reroll.patch | 27.92 KB | mondrake |
Comments
Comment #1
mitrpaka CreditAttribution: mitrpaka commentedHere is the patch to remove all uses of file_stream_wrapper_get_*, file_get_mimetype and file_get_stream_wrappers - comments lines has been left intact.
Comment #2
mitrpaka CreditAttribution: mitrpaka commentedComment #4
mitrpaka CreditAttribution: mitrpaka commentedExtension test cases re-run locally.
Comment #5
mitrpaka CreditAttribution: mitrpaka commentedComment #6
ianthomas_ukThanks for your work here.
This is OO code, can we inject stream_wrapper_manager?
OO code, injection?
I've not checked the code base after applying this patch, but if there are still references in comments then they will need to be removed too.
We will also need to link the relevant change record to this issue (or write one if it doesn't exist).
Please include an interdiff when you update a patch https://www.drupal.org/documentation/git/interdiff
Comment #7
mitrpaka CreditAttribution: mitrpaka commentedPatch re-rolled. Comments taken into account.
Comment #8
mitrpaka CreditAttribution: mitrpaka commentedComment #9
ianthomas_ukPlease ask for clarification if you don't understand a review comment - you've just used intermediate variables, which is not dependency injection. Drupal DI docs are at https://www.drupal.org/node/2133171, although I appreciate they aren't very good.
Attached is a reroll of #4, using injection in MimeTypeGuesser. I've left Update alone, as that's a bit of a special case and is already using \Drupal elsewhere in the file.
Relevant change records are linked.
Comment #11
mitrpaka CreditAttribution: mitrpaka commented@ianthomas_uk: Thank you for your help and guidance.
Updated patch to pass tests.
Comment #13
ianthomas_ukOops. Good point (it was late)
This is a test, so you can use \Drupal:: instead of $container->get() ($container has only been created on the previous line, it isn't a normal Drupal container)
Comment #14
mitrpaka CreditAttribution: mitrpaka commentedUpdated patch to pass MimeTypeGuesserTest unit test. Comments taken into account.
Comment #15
mitrpaka CreditAttribution: mitrpaka commentedfile_get_mimetype removals were missing since remove_all_uses_of-2392559-9.patch. Now added.
Comment #17
rpayanmMissing ")".
Comment #18
mitrpaka CreditAttribution: mitrpaka commentedTypo in patch. Actually one "(" too many.
Comment #19
rpayanmohhh that's right!
Comment #20
ianthomas_ukThis comment block needs to be reformatted to wrap at 80 chars
This should be injected
This should be injected
There's no need to add the explicit parameter here - it wasn't in the previous call, and will default to that value anyway.
Parameter also unnecessary
Looks OK to me other than the above. I couldn't find any remaining references to these functions.
Comment #21
rpayanmPlease review
Comment #22
ianthomas_ukJust the line length in that comment block still to do.
Comment #25
adci_contributor CreditAttribution: adci_contributor commentedJust reroll.
Comment #26
adci_contributor CreditAttribution: adci_contributor commentedAnd updated in according to #20-1.
Comment #27
ianthomas_ukThe double brackets seem to have reappeared on this line:
guess((
The interdiff in #21 must be to #17 not #18.
Comment #28
rpayanmComment #29
mondrakethis looks weird - shouldn't it be typehinted as
\Drupal\Core\StreamWrapper\StreamWrapperManager
instead? and in the constructor and the use statement as well.Comment #30
JeroenTReplaced ContainerAwareInterface with StreamWrapperManager as suggested by mondrake in #29.
Comment #31
JeroenTThis is the right interdiff.
Comment #33
JeroenTWoops, i was working on an older version of 8.0.x. Created re-roll and replaced ContainerAwareInterface with StreamWrapperManager as suggested by mondrake in #29.
Comment #34
JeroenTThis is the right patch.
Comment #35
ianthomas_ukLooks good, all comments addressed and change records linked
Comment #36
alexpottThis is example code - the use of $this does not look right.
\Drupal::service('stream_wrapper_manager')
is a little bit better.This can be injected in image.service.yml
This can be injected by implementing ContainerInjectionInterface.
Unrelated?
Comment #37
mondrake#36
1,2,3 - Done
4 - Why? It's all there to allow removal of usage of
file_get_mimetype
inwhich is in the scope. Or I miss something?
Comment #38
mondrakefile_get_mimetype()
has just been removed in #2388749: Register symfony's mime guessers if they are supported, so that piece is no longer relevant here.Rerolled, the patch is smaller now. Sorry no interdiff.
Comment #39
mondrakeComment #41
mondrake#2388749: Register symfony's mime guessers if they are supported was rolled back and postponed to later than 8.0.
Reverting changes since #38, patch in #37 is the one to review now.
Comment #42
ianthomas_ukAs shown by alexpott's question in #36 there isn't really a particularly strong link between file_get_mimetype and the other functions that we're removing. They were also covered by different CRs.
Given that we've now got a patch that just does the other functions, does it make sense to split off file_get_mimetype into its own issue?
Comment #43
ianthomas_ukremoved - double post
Comment #44
mondrake#42 fair point, let's see if #38 still runs and then we can split.
Comment #46
mondrakeCreated spin-off #2415757: Remove all uses of file_get_mimetype as suggested in #42.
#38 is still good, re-adding here to keep things clean.
Comment #47
mondrakeReroll after #2364157: Replace most existing _url calls with Url objects, only patch context changes
Comment #48
mondrakeReroll after #2050759: Move drupal_chmod and other code in file.inc to a Drupal\Core\File\FileSystem class, and changed to
StreamWrapperManagerInterface
where appropriate.Comment #49
mondrakeFound another one that got lost
Comment #51
mondrakeFixed PHPUnit test
Comment #52
mondrakeComment #53
andypostshould be replaced not removed
Comment #54
rpayanm@andypost By what should be replaced?
Because on the next line said:
IMHO I think that with that is enough.
Comment #55
JeroenTPatch no longer applied. Created re-roll.
Comment #57
JeroenTComment #58
JeroenTCreated straight reroll. Patch attached.
Comment #59
JeroenTI just realised this patch didn't need a reroll...
Comment #61
JeroenTPatch in #55 still applies.
Comment #64
JeroenTComment #65
mondrakePlain reroll.
Comment #66
mondrakeFixed a comment and a typo in the @deprecation of file_stream_wrapper_get_instance_by_uri, not sure if because of that the deprecation should now be postponed.
Also, what is the preference for services injection in tests? i.e. call \Drupal::service or $this->container->get?
The current patch does this
should it do
instead?
Comment #69
mondrakeFixed unit test.
Comment #70
mondrakeAdded beta evaluation.
Comment #71
JeroenTComment #72
Mile23The patch looks good and my IDE says it does, indeed, remove file_stream_wrapper_get_* and file_get_stream_wrappers.
Comment #73
alexpottCommitted 73585ec and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.