Skip to content

[Mailer] Fix TypeError in on SMTP error #58650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 11 commits into from

Conversation

bytestream
Copy link
Contributor

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues
License MIT

https://www.php.net/manual/en/function.error-get-last.php returns ?array. If null is returned, it produces:

Trying to access array offset on value of type null
vendor\symfony\mailer\Transport\Smtp\Stream\AbstractStream.php:91

It's unclear to me how to reproduce the issue. However, it's in our best interests to correctly handle all return types.

@xabbuh
Copy link
Member

xabbuh commented Oct 24, 2024

Did you actually experience this error or is it reported by some static analysis tool?

@bytestream
Copy link
Contributor Author

My colleague experienced it:

[2024-10-24T09:40:12.921200+00:00] local.ERROR: Trying to access array offset on value of type null {"userId":1,"exception":"[object] (ErrorException(code: 0): Trying to access array offset on value of type null at C:\\laragon\\www\\project\\vendor\\symfony\\mailer\\Transport\\Smtp\\Stream\\AbstractStream.php:91)
[stacktrace]

@xabbuh
Copy link
Member

xabbuh commented Oct 24, 2024

Which exact version of the Mailer component do they have installed?

@jshah4517
Copy link

Yes, to replicate you can test with these details (invalid username/password).

SMTP host: smtp.mailtrap.io
Port: 587
Encryption: STARTTLS
Username: a
Password: a

This is on v6.4.12 of symfony/mailer

@nicolas-grekas
Copy link
Member

That is strange. I think we should investigate why there's no warning from PHP. To me, false should be returned only when there's an error. Looks like that's not always the case, but then, what happened? We need to provide the best error message we can. Can we do better than an empty string? That's what I'm wondering.

@carsonbot carsonbot changed the title Fix TypeError in on SMTP error [Mailer] Fix TypeError in on SMTP error Oct 25, 2024
@xabbuh
Copy link
Member

xabbuh commented Oct 26, 2024

According to the documentation false is also returned when no more data can be read:

If there is no more data to read in the file pointer, then false is returned.

But I wonder how that could happen here as the method should have returned already earlier then because of the feof() at its beginning, right.

@bytestream
Copy link
Contributor Author

Closing. Cannot replicate 🤷🏼

@skylerkatz
Copy link

@bytestream and @nicolas-grekas, I just ran into this error myself, but in the vendor/symfony/mime/Part/TextPart.php file. Specifically in getBody(). If body is a file it calls @file_get_contents($this->body->getPath()). In my case, the path is a URL and returned false because the URL returned a 400 error. No error is surfaced from error_get_last but if I remove the @ an ErrorException is thrown from file_get_contents

ErrorException: file_get_contents(https://url-that-returns-a-400-response): Failed to open stream: HTTP request failed! HTTP/1.1 400 Bad Request

Obviously I should not be providing a 400 url, but the InvalidArgumentException never gets thrown, because the code is trying to pull the message off of null from error_get_last.

@xabbuh
Copy link
Member

xabbuh commented Apr 16, 2025

@skylerkatz On which exact version of the Mailer component do you experience the issue? Could this be fixed by #59404?

@skylerkatz
Copy link

I am on v7.2.3 of the mailer and v7.2.4 of symfony/mime.

@xabbuh
Copy link
Member

xabbuh commented Apr 30, 2025

@skylerkatz I tried to reproduce your issue, but I was not able to do so. My last guess is that you maybe have a custom error handler that influence the result of error_get_last(). Otherwise, please create an example application that allows to reproduce it and open a new issue to keep track of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants