Skip to content

* 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.

Sign up for GitHub

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 2 commits into from
Closed

* Added decorator method addAttachment() to allow access to createA… #3803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
067175f
* Added decorator method addAttachment() to allow access to createA…
Mar 16, 2016
ac3256f
Removed Zend Framework docblock reference, added attachment method to…
Apr 17, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions lib/internal/Magento/Framework/Mail/MessageInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,25 @@ public function setReplyTo($replyToAddress);
* @return $this
*/
public function setMessageType($type);

/**
* Creates a \Zend_Mime_Part attachment
*
* Attachment is automatically added to the mail object after creation. The
* attachment object is returned to allow for further manipulation.
*
* @param string $body
* @param string $mimeType
* @param string $disposition
* @param string $encoding
* @param string $filename OPTIONAL A filename for the attachment
* @return \Zend_Mime_Part Newly created \Zend_Mime_Part object (to allow
* advanced settings)
*/
public function createAttachment(
$body,
$mimeType = \Zend_Mime::TYPE_OCTETSTREAM,
$disposition = \Zend_Mime::DISPOSITION_ATTACHMENT,
$encoding = \Zend_Mime::ENCODING_BASE64,
$filename = null);
}
22 changes: 22 additions & 0 deletions lib/internal/Magento/Framework/Mail/Template/TransportBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,28 @@ public function setTemplateOptions($templateOptions)
return $this;
}

/**
* Creates a Zend_Mime_Part attachment and attaches it to the current message scope.
* Attachment is automatically added to the mail object after creation.
*
* @param string $body
* @param string $mimeType
* @param string $disposition
* @param string $encoding
* @param string $filename A filename for the attachment
* @return $this
*/
public function addAttachment(
$body,
$mimeType = \Zend_Mime::TYPE_OCTETSTREAM,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not expose that Zend Framework is used in implementation

$disposition = \Zend_Mime::DISPOSITION_ATTACHMENT,
$encoding = \Zend_Mime::ENCODING_BASE64,
$filename = null
) {
$this->message->createAttachment($body, $mimeType, $disposition, $encoding, $filename);
Copy link

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

Copy link
Author

@der-workfloh der-workfloh Apr 17, 2016 •
edited
Loading

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.

Copy link
Contributor

@adragus-inviqa adragus-inviqa Apr 17, 2016 •
edited
Loading

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.).

Copy link
Author

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?

Copy link
Contributor

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.

return $this;
}

/**
* Get mail transport
*
Expand Down