Bug fix for Issue #529 #1026

Merged
merged 4 commits into from Mar 29, 2012

Projects

None yet

4 participants

@dianaprajescu
Contributor

JMail is ignoring name parameter on addAttachment.

@realityking
Member

You have a few code style error you'll need to fix: http://developer.joomla.org/pulls/pulls/1026.html

@realityking realityking commented on an outdated diff Mar 23, 2012
libraries/joomla/mail/mail.php
{
- parent::AddAttachment($file, $name, $encoding, $type);
+ throw new RuntimeException("The number of attachments must be equal with the number of name");
+ }
+
+ for ($i=0; $i<sizeof($attachment); $i++)
@realityking
realityking Mar 23, 2012 Member

I think we should use foreach($attachment $key => $file) instead of the for loop and document that $attachment and $name have to have the same keys. This way the keys don't have to be numerical and without gaps.

@realityking realityking commented on an outdated diff Mar 23, 2012
libraries/joomla/mail/mail.php
{
- parent::AddAttachment($file, $name, $encoding, $type);
+ throw new RuntimeException("The number of attachments must be equal with the number of name");
+ }
+
+ for ($i=0; $i<sizeof($attachment); $i++)
+ {
+ if (!empty($name))
+ {
+ parent::AddAttachment($attachment[$i], $name[$i], $encoding, $type);
+ }
+ else
+ {
+ parent::AddAttachment($attachment[$i], $encoding, $type);
@realityking
realityking Mar 23, 2012 Member

This doesn't fit the method signature of AddAttachment():

public function AddAttachment($path, $name = '', $encoding = 'base64', $type = 'application/octet-stream') {
@elinw
Contributor
elinw commented Mar 25, 2012

Hi Diana,

Even though the rest of the test isn't implemented, would you add a test for AddAttachement?
https://github.com/joomla/joomla-platform/blob/staging/tests/suites/unit/joomla/mail/JMailTest.php

@chdemko chdemko merged commit c09a845 into joomla:staging Mar 29, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment