- Issue created by @mfb
- First commit to issue fork.
- ๐ฎ๐ณIndia chandansha
Hello @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!!
- ๐บ๐ธUnited States mfb San Francisco
@Chandansha Oops I forgot to mention configuring the field formatter in the steps to reproduce, that is probably what you were missing
- @chandansha opened merge request.
- Status changed to Needs review
6 months ago 12:21pm 7 August 2024 - ๐ฎ๐ณIndia chandansha
I have fixed deprecated function error.
Please review.
Thanks!! - Status changed to Needs work
6 months ago 1:23pm 7 August 2024 - ๐บ๐ธUnited States smustgrave
Research needs to be done to figure out why it's null. Putting just a simple check could be masking a larger issue.
- ๐บ๐ธUnited States mfb San Francisco
@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.
- ๐ง๐ทBrazil charlliequadros
Hi @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.
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.
- ๐บ๐ธUnited States smustgrave
Possibly will need to have test coverage to handle when unknown file type is sent.
- ๐บ๐ธUnited States mfb San Francisco
Looks 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.
- ๐ง๐ทBrazil charlliequadros
Hi @Chandansha,
I added the check that @mfb requested in the function where the error occurs. I hope you don't mind. - ๐ญ๐บHungary mxr576 Hungary
This 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.
- Status changed to Needs review
6 months ago 10:00pm 9 August 2024 - ๐บ๐ธUnited States mfb San Francisco
Added this scenario to the existing functional test
- Status changed to RTBC
6 months ago 10:59pm 9 August 2024 - ๐บ๐ธUnited States smustgrave
Ran 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.
- Status changed to Needs work
6 months ago 4:19am 10 August 2024 - ๐ณ๐ฟNew Zealand quietone
I 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 โ .
- Status changed to Needs review
6 months ago 5:47am 10 August 2024 - Status changed to RTBC
6 months ago 2:55pm 11 August 2024 - ๐บ๐ธUnited States smustgrave
Thanks @mxr576 for taking care of that so quickly!
I 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.
- ๐บ๐ธUnited States mfb San Francisco
@blb I created a followup issue @ ๐ Some file extensions in field.field.media.document.field_media_document.yml are missing from ExtensionMimeTypeGuesser Active
- ๐ซ๐ทFrance Grimreaper France ๐ซ๐ท
Hi,
Thanks for the work here!
Encountered the same issue and MR fixes the bug on Core 10.3.2
- ๐บ๐ธUnited States dalemoore
Just encountered the same error message 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.
- ๐ฆ๐บAustralia kim.pepper ๐โโ๏ธ๐ฆ๐บSydney, Australia
We 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. - @kimpepper opened merge request.
- ๐ฆ๐บAustralia kim.pepper ๐โโ๏ธ๐ฆ๐บSydney, Australia
Created a MR that does #29
- ๐บ๐ธUnited States mfb San Francisco
@kim.pepper But in this case, we have a "fake" file:
'fakedFile.' . $extension
So, we only want to retrieve a MIME type based on the extension, not anything else thatfile.mime_type.guesser
might use. Thus onlyfile.mime_type.guesser.extension
is used directly. - ๐ฆ๐บAustralia kim.pepper ๐โโ๏ธ๐ฆ๐บSydney, Australia
Ah you are right. Back to RTBC for MR!9114
- ๐ฆ๐บAustralia kim.pepper ๐โโ๏ธ๐ฆ๐บSydney, Australia
I created a task ๐ Create a method for getting the MIME type for a file extension from a map Active that should make looking up the MIME type for an extension less hacky.
- ๐ฆ๐บAustralia kim.pepper ๐โโ๏ธ๐ฆ๐บSydney, Australia
We don't need a re-roll @peter_serfozo as we are using the MR !9114
- ๐ณ๐ฟNew Zealand quietone
I read the IS, comments and the MR. There are no unanswered questions and everything is in order here.
Leaving at RTBC
- ๐ซ๐ทFrance PhilY ๐ช๐บ๐ซ๐ท Paris, France
MR !9114 works for me using Drupal 10.3.6
- ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
I 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. - ๐บ๐ธUnited States mfb San Francisco
As I mentioned in #9, silently ignoring the null rather than logging a warning seems fine to me.
- ๐บ๐ธUnited States smustgrave
Resolved some of the threads, from what I can tell the feedback has been addressed.
- ๐ณ๐ฟNew Zealand quietone
One remaining question here is #40. I'll leave that to another committer to respond to.
- ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
Committed 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!
-
alexpott โ
committed 51439626 on 10.4.x
Issue #3466462 by mfb, chandansha, kim.pepper, charlliequadros,...
-
alexpott โ
committed 51439626 on 10.4.x
-
alexpott โ
committed 1f1970a9 on 10.5.x
Issue #3466462 by mfb, chandansha, kim.pepper, charlliequadros,...
-
alexpott โ
committed 1f1970a9 on 10.5.x
-
alexpott โ
committed f96fff0d on 11.1.x
Issue #3466462 by mfb, chandansha, kim.pepper, charlliequadros,...
-
alexpott โ
committed f96fff0d on 11.1.x
-
alexpott โ
committed ec3fedad on 11.x
Issue #3466462 by mfb, chandansha, kim.pepper, charlliequadros,...
-
alexpott โ
committed ec3fedad on 11.x
- ๐บ๐ธUnited States smustgrave
Weird I clearly see it was marked fixed but didnโt stick
Automatically closed - issue fixed for 2 weeks with no activity.