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

Remove unused variable #212

Open
wants to merge 4 commits into
base: 2.18.x
Choose a base branch
from
Open

Remove unused variable #212

wants to merge 4 commits into from

Conversation

holtkamp
Copy link

Signed-off-by: Menno Holtkamp holtkamp@users.noreply.github.com

Q A
Documentation yes
Bugfix no
BC Break no
New Feature no
RFC no
QA no

renovate bot and others added 2 commits September 24, 2022 01:04
Signed-off-by: Renovate Bot <bot@renovateapp.com>
Signed-off-by: Menno Holtkamp <holtkamp@users.noreply.github.com>
@froschdesign froschdesign added Bug Something isn't working Documentation labels Sep 28, 2022
@froschdesign froschdesign added this to the 2.18.1 milestone Sep 28, 2022
@froschdesign
Copy link
Member

froschdesign commented Sep 28, 2022

@holtkamp
Please use the branch 2.18.x for a bug fix.
Thanks in advance! 👍🏻

More information on this topic can be found in contributing guidelines:

What branch to issue the pull request against?

Which branch should you issue a pull request against?

  • For fixes against the stable release, issue the pull request against the release branch matching the minor release you want to fix.
  • For new features, or fixes that introduce new elements to the public API (such as new public methods or properties), issue the pull request against the default branch.

@holtkamp
Copy link
Author

holtkamp commented Sep 28, 2022

#212 (comment)

What do you mean with:

See: https://docs.laminas.dev/laminas-mail/message/attachments/#multipartalternative-emails-with-attachments

That $body variable is set twice. The first time is not needed.

Screenshot 2022-09-28 at 20 58 44

Additionally, the example is incorrect, it is missing this part which sets the content type of the "alternative" multipart part:

https://github.com/laminas/laminas-mail/blob/2.19.x/test/MessageTest.php#L812-L814

Does this require a separate issue?

@holtkamp holtkamp changed the base branch from 2.19.x to 2.18.x September 28, 2022 19:05
@holtkamp
Copy link
Author

FYI: changed target branch from 2.19.x to 2.18.x

@froschdesign
Copy link
Member

@holtkamp

That $body variable is set twice. The first time is not needed.

Correct. I have only added the link as a reference.

@froschdesign
Copy link
Member

FYI: changed target branch from 2.19.x to 2.18.x

A rebase is needed.

@froschdesign
Copy link
Member

Additionally, the example is incorrect, it is missing this part which sets the content type of the "alternative" multipart part:

https://github.com/laminas/laminas-mail/blob/2.19.x/test/MessageTest.php#L812-L814

Does this require a separate issue?

No, add it to this pull request as a separate commit.

Signed-off-by: Menno Holtkamp <holtkamp@users.noreply.github.com>
@holtkamp
Copy link
Author

holtkamp commented Sep 28, 2022

A rebase is needed.

Tried to rebase on 2.18.x, but it seems "everything is up-to-date".

Could be my lack of GIT skills, I used this as reference https://stackoverflow.com/questions/7929369/how-to-rebase-local-branch-onto-remote-master

Feel free to edit my PR if applicable.

(Squashing commits into a single commit is also something I still struggle with and that you will probably request me to do...)

Signed-off-by: Menno Holtkamp <holtkamp@users.noreply.github.com>
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.

3 participants