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

[Mime] Add NamedAddress::fromString #33091

Open
wants to merge 14 commits into
base: 4.4
from

Conversation

@gisostallenberg
Copy link
Contributor

commented Aug 9, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #33086
License MIT
Doc PR symfony/symfony-docs#12128

This will allow to create a NamedAddress from a string such as 'Name name@example.com'
Example:

$address = NamedAddress::fromString("Name <name@example.com>");
Add NamedAddress::fromString
This will allow to create a NamedAddress from a string such as 'Name <name@example.com>'
@Shkarsardar

This comment has been minimized.

Copy link

commented Aug 9, 2019

can you explain it? how the feature should work

@gisostallenberg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

src/Symfony/Component/Mime/CHANGELOG.md Outdated Show resolved Hide resolved
src/Symfony/Component/Mime/NamedAddress.php Outdated Show resolved Hide resolved
src/Symfony/Component/Mime/NamedAddress.php Outdated Show resolved Hide resolved
src/Symfony/Component/Mime/NamedAddress.php Outdated Show resolved Hide resolved
src/Symfony/Component/Mime/NamedAddress.php Outdated Show resolved Hide resolved
src/Symfony/Component/Mime/NamedAddress.php Outdated Show resolved Hide resolved
src/Symfony/Component/Mime/NamedAddress.php Outdated Show resolved Hide resolved

@Tobion Tobion added the Mime label Aug 9, 2019

@chalasr chalasr added this to the next milestone Aug 9, 2019

@nicolas-grekas nicolas-grekas changed the title Add NamedAddress::fromString [Mime] Add NamedAddress::fromString Aug 11, 2019

gisostallenberg added some commits Aug 12, 2019

Add test to construct from NamedAddress::toString
Fixed having no name in string and adjusted test for that
Update regex
Update the FROM_STRING_PATTERN to match the first < and last > as addrSpec.
Also expanded tests to cover more samples
@sstok

sstok approved these changes Aug 12, 2019

src/Symfony/Component/Mime/NamedAddress.php Outdated Show resolved Hide resolved
src/Symfony/Component/Mime/NamedAddress.php Outdated Show resolved Hide resolved
@gisostallenberg

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Thanks so much for all the effort you put in checking this.

public static function fromString(string $string): self
{
if (false === strpos($string, '<')) {
return new self($string, '');

This comment has been minimized.

Copy link
@fabpot

fabpot Aug 19, 2019

Member

What about returning a simple Address instance here instead?

This comment has been minimized.

Copy link
@OskarStark

OskarStark Aug 19, 2019

Contributor

I am against this, because in this case fromString() would have 2 return types NamedAddress|Address 👎

This comment has been minimized.

Copy link
@fabpot

fabpot Aug 19, 2019

Member

But returning a NamedAddress is a lie, as there is no name. And the return type should indeed be changed, but to Address only (as NamedAddress is a subclass of Address). What we care about is having an Address in your code (the fact that it can be a NamedAddress is an implementation details).

Another possibility would be to remove the distinction between Address and NamedAddress and only have Address which would support named addresses.

This comment has been minimized.

Copy link
@gisostallenberg

gisostallenberg Aug 19, 2019

Author Contributor

I also did not like having this method returning two kinds of classes. It could also result in an InvalidArgumentException as far as I'm concerned.

This comment has been minimized.

Copy link
@fabpot

fabpot Aug 19, 2019

Member

I think throwing an exception is not an option as the use case is configuration, where you don't really know what the user is going to use.

This comment has been minimized.

Copy link
@gisostallenberg

gisostallenberg Aug 19, 2019

Author Contributor

But...

$address = new NamedAddress('example@example.com', '');

is allowed at the moment, maybe that should also not be the case?

This comment has been minimized.

Copy link
@sstok

sstok Aug 19, 2019

Contributor

Another possibility would be to remove the distinction between Address and NamedAddress and only have Address which would support named addresses.

👍

This comment has been minimized.

Copy link
@gisostallenberg

gisostallenberg Aug 19, 2019

Author Contributor

Another possibility would be to remove the distinction between Address and NamedAddress and only have Address which would support named addresses.

Would that be a think to do in this PR, or create a separate PR for?

This comment has been minimized.

Copy link
@OskarStark

OskarStark Aug 19, 2019

Contributor

What we care about is having an Address in your code (the fact that it can be a NamedAddress is an implementation details).

This is true, but named constructors are not meant to return anything else than the static class itself as AFAIK. It would be strange to me:

public function myMethod(NamedAddress $foo) {
    // ...
}

and get an exception when calling it like this, because its not a NamedAdress:

$this->myMethod(NamedAddress::fromString(.....))
src/Symfony/Component/Mime/NamedAddress.php Outdated Show resolved Hide resolved
src/Symfony/Component/Mime/NamedAddress.php Outdated Show resolved Hide resolved
src/Symfony/Component/Mime/NamedAddress.php Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.