Skip to content
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

Add support for headers in native mailer #304

Merged
merged 2 commits into from Jun 20, 2019

Conversation

nalysius
Copy link
Contributor

The native mailer is able to have additional headers, but it
wasn't possible to give it any headers in the configuration.
Swift mailer may have a content_type key but not native mailer.

A new "headers" key is now allowed in the handler configuration, so a
list of headers may be given to the handler. Only native mailer supports
it.

Related to #272

@lyrixx lyrixx added the Feature label May 22, 2019
@lyrixx
Copy link
Member

lyrixx commented Jun 3, 2019

hello @anthonybocci. Your code is not valid. Could you fix it? (It generates a fatal error, see the CI)

@nalysius
Copy link
Contributor Author

nalysius commented Jun 3, 2019

Hello @lyrixx. I didn't see the errors on the page, sorry. I'll fix it. For now the XSD seems not to be valid, my complex type is not accepted.

@lyrixx
Copy link
Member

lyrixx commented Jun 4, 2019

There is this error too:

PHP Fatal error:  Call to undefined method Symfony\Component\Config\Definition\Builder\ArrayNodeDefinition::scalarPrototype() in /home/travis/build/symfony/monolog-bundle/DependencyInjection/Configuration.php on line 547

https://travis-ci.org/symfony/monolog-bundle/jobs/534250452

@fabpot Symfony 2.8 is not maintained anymore. I think we could drop support for it. WDYT ?

@fabpot
Copy link
Member

fabpot commented Jun 4, 2019

I would indeed drop 2.8 support.

@lyrixx
Copy link
Member

lyrixx commented Jun 4, 2019

See #306 for the drop

@nalysius
Copy link
Contributor Author

nalysius commented Jun 4, 2019

Thank you @lyrixx for pointing this out, I'll try to have a look this week.

@lyrixx
Copy link
Member

lyrixx commented Jun 11, 2019

Hi. I dropped support for SF <3.4. Could you rebase your PR? Thanks

The native mailer is able to have additional headers, but it
wasn't possible to give it any headers in the configuration.
Swift mailer may have a content_type key but not native mailer.

A new "headers" key is now allowed in the handler configuration, so a
list of headers may be given to the handler. Only native mailer supports
it.

Related to symfony#272
@lyrixx lyrixx force-pushed the native-mailer-support-headers branch from 7c344c7 to 7ecbc9e Compare June 11, 2019 15:43
@lyrixx
Copy link
Member

lyrixx commented Jun 11, 2019

I have rebased this PR for you

@lyrixx
Copy link
Member

lyrixx commented Jun 11, 2019

The XML is still invalid.
Could you fix it please:

1) Symfony\Bundle\MonologBundle\Tests\DependencyInjection\XmlMonologExtensionTest::testLoadWithSeveralHandlers
Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: Unable to parse file "/home/travis/build/symfony/monolog-bundle/Tests/DependencyInjection/Fixtures/xml/multiple_handlers.xml".

@nalysius
Copy link
Contributor Author

Hi, the error is in fact attribute decl. 'headers', attribute 'type': The QName value '{http://symfony.com/schema/dic/monolog}headers' does not resolve to a(n) simple type definition. (in file:////home/anthony/Documents/programming/php/monolog-bundle/DependencyInjection/../Resources/config/schema//monolog-1.0.xsd, because I have created headers in the file monolog-1.0.xsd, I will try to fix this issue today, but I don't understand where my type is invalid yet.

@lyrixx lyrixx force-pushed the native-mailer-support-headers branch from fd6db6f to b64cb29 Compare June 12, 2019 08:53
@lyrixx
Copy link
Member

lyrixx commented Jun 12, 2019

@anthonybocci I finished the PR for you. I pushed in your branch, so if you need to change something, be careful.

I have one little question: did you test your work in a real application? I'm asking because of this change
I made.

            if (!empty($handler['headers'])) {
-                $definition->addMethodCall('addHeader', $handler['headers']);
+                $definition->addMethodCall('addHeader', [$handler['headers']]);
            }

I did not test in a real application, but this is how Definition::addMethodCall() works. I may be wrong, but I don't think.

@nalysius
Copy link
Contributor Author

Thank you @lyrixx, I was far from the result.
I did test in a real application during my development (before trying to fix the unit tests), and I successfully received the email as HTML thanks to the header. I'm not familiar with Definition that's why I make the error in addMethodCall, sorry.
Do you need me to test the branch, or is it ok for you? If it's ok for you, it is for me.

@lyrixx
Copy link
Member

lyrixx commented Jun 12, 2019

If you can test it, it would be very nice. Thanks.

@nalysius
Copy link
Contributor Author

For sure, I will test it this evening.

@nalysius
Copy link
Contributor Author

@lyrixx I just tested. It is working if I use the following form:

headers:
  - "Content-Type: text/html"

But it's not working when I use the following form:

headers:
  content-type: text/html

In the second case only the value is added to the content, not the key.
The first case works and I can combine it with the formatter key so the mail is prettier.

In my commits I tried to implement the first case because it allows to use any chars, and I was not sure if every char was allowed in a YAML key.

@lyrixx
Copy link
Member

lyrixx commented Jun 12, 2019

It's weird because according to the test cast it should be ok. I'll look at it in a real app tomorrow. Thanks for your feedbacks

@nalysius
Copy link
Contributor Author

If it helps, here is the demonstration of what I encounter when I test:

config/packages/dev/monolog.yaml

monolog:
    handlers:
        main:
            type: fingers_crossed
            action_level: debug
            handler: nested
            excluded_http_codes: [403, 404] 
            max_files: 10
        deduplicated:
            type:    deduplication
            handler: mail
        nested:
            type: stream
            path: "%kernel.logs_dir%/%kernel.environment%.log"
            level: debug
            handler: deduplicated
        mail:
            type:       native_mailer
            from_email: 'logger@email.com'
            to_email:   ['dev@email.com']
            subject:    'Mail Log'
            level:      error
            headers:
                Content-Type: "text/html"

Here is the beginning of the email's source:

Return-Path: <...>
Received: from ...
        for <...>
        (version=... cipher=... bits=...);
        ...
Received: by ...
	id ...; ...
To: ...
Subject: ...
From: ...
Date: ...

text/html
Content-type: text/plain; charset=utf-8

The interesting part is the text/html at the end of the message. It's the config value, but the header name is not here. If I change my monolog.yaml a bit it works:

headers:
    - "Content-Type: text/html"

But your tests are done as if it we were using the first structure.

@lyrixx
Copy link
Member

lyrixx commented Jun 19, 2019

Actually, Symfony normalize the keys. So using:

headers:
  content-type: text/html

is a bad idea.


You should use the following configuration instead

headers:
  - "content-type: text/html"

@lyrixx
Copy link
Member

lyrixx commented Jun 19, 2019

I updated the PR to reflect this decision

@lyrixx lyrixx force-pushed the native-mailer-support-headers branch from b64cb29 to 19eaa0f Compare June 19, 2019 14:34
@nalysius
Copy link
Contributor Author

@lyrixx Both configurations seem to be the same in your previous comment.

@lyrixx
Copy link
Member

lyrixx commented Jun 19, 2019

ahaha, sorry. I edited my comment. Thanks

@nalysius
Copy link
Contributor Author

Haha I read it twice to be sure. I'm ok with the current configuration, it already worked. Do you need me to test it again?

@lyrixx
Copy link
Member

lyrixx commented Jun 19, 2019

I test it :) it should be OK :)

@lyrixx
Copy link
Member

lyrixx commented Jun 19, 2019

But if you can, it would be nice !

@nalysius
Copy link
Contributor Author

I just tested it, I receive the email as HTML, it's good for me :)

@lyrixx
Copy link
Member

lyrixx commented Jun 20, 2019

Thanks for your work on this new feature!

@lyrixx lyrixx merged commit 19eaa0f into symfony:master Jun 20, 2019
lyrixx added a commit that referenced this pull request Jun 20, 2019
…lyrixx)

This PR was merged into the 3.x-dev branch.

Discussion
----------

Add support for headers in native mailer

The native mailer is able to have additional headers, but it
wasn't possible to give it any headers in the configuration.
Swift mailer may have a content_type key but not native mailer.

A new "headers" key is now allowed in the handler configuration, so a
list of headers may be given to the handler. Only native mailer supports
it.

Related to #272

Commits
-------

19eaa0f Fix 'Add support for headers in native mailer'
7ecbc9e Add support for headers in native mailer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants