Created the patch file for 9114 PR instead of using PR in composer.json file
| Comment | File | Size | Author |
|---|---|---|---|
| #55 | 3466462-18-02.patch | 8.4 KB | dhavalpanchal |
Issue fork drupal-3466462
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #3
chandansha commentedHello @mfb ,
i did not face same error like you can say. could please verify my step. if i am wrong please guide me to reproduced error.
1. setup project using 3466462-deprecated-function-explode this branch.
2. create file field in basic page and set file extension foobaz, mp3.
3. create node and upload mp3 file.
4. open node in front page. (i can't see error).
5. Again i try another way like create view for basic page and render file field and open that page still i can't see error.
Can you please provide proper steps to reproduced error
Thanks!!
Comment #4
mfbComment #5
mfb@Chandansha Oops I forgot to mention configuring the field formatter in the steps to reproduce, that is probably what you were missing
Comment #7
chandansha commentedI have fixed deprecated function error.
Please review.
Thanks!!
Comment #8
smustgrave commentedResearch needs to be done to figure out why it's null. Putting just a simple check could be masking a larger issue.
Comment #9
mfb@smustgrave the premise is configuring an unknown file extension, so I think it's reasonable for the mime type to be null.
And silently ignoring/masking the null seems fine to me, unless folks think it's important enough to log a warning that an extension with unknown mime type was configured.
Left at needs work since I suggested changes on the merge request.
Comment #10
charlliequadros commentedHi @smustgrave
The issue occurs because when an unknown file type is created, it doesn't find the MIME type, resulting in a null return from this function.
https://git.drupalcode.org/project/drupal/-/blob/11.0.0/core/lib/Drupal/Core/File/MimeType/ExtensionMimeTypeGuesser.php#L929
One solution would be to set 'application/octet-stream' as the MIME type for unknown file types. This would prevent the error from occurring. However, I'm not sure if this would be the best approach or if the current implementation in the MR is sufficient, as it already resolves the problem.
Comment #11
smustgrave commentedPossibly will need to have test coverage to handle when unknown file type is sent.
Comment #12
mfbLooks like MimeTypeGuesser::guessMimeType() returns 'application/octet-stream' if all of the registered guesser(s) have returned null. But ExtensionMimeTypeGuesser::guessMimeType(), being a specific guesser, returns null.
In this case, the specific guesser is being called directly, which is a bit odd, but doing it this way means a null mime type is possible.
So yes, just handling this possible null seems like the easiest solution.
Comment #13
charlliequadros commentedHi @Chandansha,
I added the check that @mfb requested in the function where the error occurs. I hope you don't mind.
Comment #14
morvaim commentedComment #15
morvaim commentedComment #16
mxr576Comment #17
mxr576This issue was most likely caused by fixing this other one... rolling back the change introduced by that issue probably would not be a good fix, since it addressed and important mismatch between expected behavior vs. actual implementation.
Comment #18
mfbAdded this scenario to the existing functional test
Comment #19
smustgrave commentedRan test-only feature https://git.drupalcode.org/issue/drupal-3466462/-/jobs/2398791 and shows the coverage and code change seems straight forward.
Will go ahead and mark.
Comment #20
quietone commentedI have only noticed the title here.
The title should be a description of what is being fixed or improved. The title is used as the git commit message so it should be meaningful an concise. See List of issue fields.
Comment #21
mxr576Comment #22
mxr576Comment #23
smustgrave commentedThanks @mxr576 for taking care of that so quickly!
Comment #24
blb commentedI had this problem with the documents media type when was in the UI for Manager display it had this error and i was prevented from making changes. I mention this here in case anyone encounters the same.
'Deprecated function: explode(): Passing null to parameter #2 ($string) of type string is deprecated in Drupal\file\Plugin\Field\FieldFormatter\FileMediaFormatterBase::mimeTypeApplies() (line 171 of core/modules/file/src/Plugin/Field/FieldFormatter/FileMediaFormatterBase.php)' was repeated many times. (on D 10.3.2)
It seems by default the documents type has a long list of acceptable suffixes:
file_extensions: 'txt rtf doc docx ppt pptx xls xlsx pdf odf odg odp ods odt fodt fods fodp fodg key numbers pages'
replacing with the shorter list: txt, doc, docx, pdf -- solves the problem. maybe this should be looked at.
Comment #25
mfb@blb I created a followup issue @ #3467501: File extensions "numbers" and "pages" in field.field.media.document.field_media_document.yml are missing from ExtensionMimeTypeGuesser
Comment #26
grimreaperHi,
Thanks for the work here!
Encountered the same issue and MR fixes the bug on Core 10.3.2
Comment #27
mfbHiding out of date patch
Comment #28
dalemoore commentedJust encountered the same error message on a new site when adding a new view mode to the Document media type; paired the allowed list in the field to "txt, rtf, doc, docx, ppt, pptx, xls, xlsx, pdf" and it bypasses for now. Just deleting "numbers" and "pages" from the list didn't fix so must be others not in there.
Comment #29
kim.pepperWe shouldn't be calling
ExtensionMimeTypeGuesser(service id'file.mime_type.guesser.extension') directly, but instead using'file.mime_type.guesser'which is a service collector and returns a default of 'application/octet-stream' if none are found.Comment #31
kim.pepperCreated a MR that does #29
Comment #32
mfb@kim.pepper But in this case, we have a "fake" file:
'fakedFile.' . $extensionSo, we only want to retrieve a MIME type based on the extension, not anything else thatfile.mime_type.guessermight use. Thus onlyfile.mime_type.guesser.extensionis used directly.Comment #34
kim.pepperAh you are right. Back to RTBC for MR!9114
Comment #35
kim.pepperI created a task #3476294: Create a method for getting the MIME type for a file extension from a map that should make looking up the MIME type for an extension less hacky.
Comment #36
peter_serfozo commentedEdited: patch from the MR at commit https://git.drupalcode.org/project/drupal/-/merge_requests/9114/diffs?co...
Comment #37
kim.pepperWe don't need a re-roll @peter_serfozo as we are using the MR !9114
Edit: ah unless you are using composer patches.
Comment #38
quietone commentedI read the IS, comments and the MR. There are no unanswered questions and everything is in order here.
Leaving at RTBC
Comment #39
philyMR !9114 works for me using Drupal 10.3.6
Comment #40
larowlanI think we need to also handle a possible NULL value in a second place in the formatter.
We should also consider logging the invalid file extension rather than silently ignoring it.
Comment #41
mfbAs I mentioned in #9, silently ignoring the null rather than logging a warning seems fine to me.
Comment #42
smustgrave commentedResolved some of the threads, from what I can tell the feedback has been addressed.
Comment #43
quietone commentedOne remaining question here is #40. I'll leave that to another committer to respond to.
Comment #44
alexpottCommitted and pushed ec3fedad1d8 to 11.x and f96fff0dad9 to 11.1.x and 1f1970a9bf8 to 10.5.x and 51439626866 to 10.4.x. Thanks!
Comment #49
alexpottComment #52
smustgrave commentedWeird I clearly see it was marked fixed but didn’t stick
Comment #54
yevko commentedIndeed, this is not in 11.1.1
Comment #55
dhavalpanchal commentedCreating patch file for Drupal 10.4.1 and PHP 8.3.