-
Notifications
You must be signed in to change notification settings - Fork 9.4k
* Added decorator method addAttachment() to allow access to createA… #3803
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
* Added decorator method addAttachment() to allow access to createA… #3803
Conversation
…tachment() method form Zend_Mail component
$encoding = \Zend_Mime::ENCODING_BASE64, | ||
$filename = null | ||
) { | ||
$this->message->createAttachment($body, $mimeType, $disposition, $encoding, $filename); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As declared in constructor $message
is MessageInterface
which do not have method createAttachment
. We can not add it now as that will be backward incompatible change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can not add it now as that will be backward incompatible change
First, I do not understand why this should be a backward incompatible change due to the only extensible kind of change and no change on the existing code (new method, no override).
Second, the internal message entity attribute is refering to the Magento2 Message entity and not to the Message interface, which is for me a bug itself (see line 71 of Magento2 TransportBuilder).
The Magento2 Message entity is extending from the Zend_Mail entity. I provide the solution to add this method to the Magento2 Message interface, so this issue can be resolved.
I do not see any more conflict with Magento2 architecture concept or backward compatibilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, I do not understand why this should be a backward incompatible change due to the only extensible kind of change and no change on the existing code (new method, no override).
When you add or remove a new method on an interface, you're breaking every other 3rd party implementation of that interface. The high level API is a hard thing to change without pissing people off.
@vkublytskyi is not refering to internal classes, but other people's implementation of that interface.
And I agree with him that you're leaking implementation details (\Zend_Mime
). If I want it to be as abstract as possible (as an interface/API should be), I usually try to test it with 3 different implementations (e.g. Zend, Symfony etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adragus-inviqa thank you for the explanation, the problem is more clear to me now.
May the problem be resolved by adding a new message interface (extending the current one) especially for the injection to the transport builder constructor providing the new method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, there are a couple of ways to tackle this.
One Java-ish method would be to create a new MessageInterface2
, which extends the original. There you can add any method you want, as it's not used by anyone. In due time, the original would be deprecated, then removed in favour of the new one.
Another way would be the Decorator pattern (MessageWithAttachment
or something).
Either way, you would still have the problem of leaking implementation details.
You'll need to put more thought in this and create a new MessageAttachment
interface which should satisfy most of the popular frameworks out there. From what I can see, it usually boils down to filename, encoding, content type and disposition. This is the case with \Zend_Mime
and Symfony
's own builder: http://swiftmailer.org/docs/messages.html#attaching-files. This functionality lends itself perfectly for abstraction. So do create such an interface with methods like getDisposition()
, getFileName()
etc., then you can create a Zend_Mime
-compatible implementation and set a preference to that in M2's DI. Most use SwiftMailer anyway, so if you consider that, you're covering most of the userbase.
… Message Interface
Internal ticket: MAGETWO-52763 |
Closing, while comments are not resolved. Please feel free to reopen when it's ready. |
Enables attachment support for TransportBuilder: