Skip to content

[Mailer][DX][RFC] Rename mailer bridge transport classes #32609

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

Jump to bottom
Merged
merged 1 commit into from
Jul 25, 2019
Merged

[Mailer][DX][RFC] Rename mailer bridge transport classes #32609

merged 1 commit into from
Jul 25, 2019

Conversation

Koc
Copy link
Contributor

@Koc Koc commented Jul 18, 2019 •
edited
Loading

Q A
Branch? 4.4
Bug fix? yno
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

During working on #31946 I realized how painful to work with multiple classes which has same name. Nice article by @TomasVotruba with explanation of problems with such approach.
Built on top of #32608 , so only 2nd commit is actual.

Also I've changed namespaces to make bridge structure much simpler and be linear. All classes located on same level now. See how bridge looks like now.

Now in RFC state to get approve for such king of changes and update all other bridges.

@carsonbot carsonbot added Status: Needs Review DX DX = Developer eXperience (anything that improves the experience of using Symfony) RFC RFC = Request For Comments (proposals about features that you want to be discussed) BC Break labels Jul 18, 2019
@fabpot
Copy link
Member

fabpot commented Jul 19, 2019

The renaming looks like a good idea to me 👍

@nicolas-grekas nicolas-grekas added this to the next milestone Jul 19, 2019
@nicolas-grekas
Copy link
Member

Please update the CHANGELOG file.

@Koc Koc force-pushed the rename-mailer-bridge-classes branch 4 times, most recently from 5db5558 to b9dd600 Compare July 22, 2019 20:10
@Koc
Copy link
Contributor Author

Koc commented Jul 22, 2019 •
edited
Loading

Branch rebased, PR is ready for review.

1st commit about renaming classes in bridges
2nd about some cleanup of built transports: abstract http and api classes moved under transport, in same time transport implementations moved under corresponsing sub-namespaces. So now Transport sub-namespace contains only abstractions + Dsn

@fabpot
Copy link
Member

fabpot commented Jul 23, 2019

Having shorter namespaces is also a goal. I would revert the creation of the Chain, Null, and Sendmail namespaces.

@Koc
Copy link
Contributor Author

Koc commented Jul 23, 2019 •
edited
Loading

But in this case we got mess inside Transport subnamespace: a lot of classes, abstractions mixed with implementations.

There are 2 PRs with new transports (file, in-memory) that contribute to same Transport Subnamespace.

@Koc
Copy link
Contributor Author

Koc commented Jul 23, 2019

изображение
vs
изображение

@fabpot
Copy link
Member

fabpot commented Jul 24, 2019

But File and InMemory transports do not exist (and will probably never exist).

@Koc
Copy link
Contributor Author

Koc commented Jul 24, 2019

There are pending PRs about that: #31947, #32409

@fabpot
Copy link
Member

fabpot commented Jul 24, 2019

Haven't had time to have a look at them yet, but I don't think they will be merged.

@Koc Koc force-pushed the rename-mailer-bridge-classes branch from b9dd600 to fe403c2 Compare July 24, 2019 20:40
@Koc
Copy link
Contributor Author

Koc commented Jul 24, 2019

Creation of the Chain, Null, and Sendmail namespaces has been reverted.

@fabpot fabpot force-pushed the rename-mailer-bridge-classes branch from f1cf336 to eda4f01 Compare July 25, 2019 09:34
@fabpot
Copy link
Member

fabpot commented Jul 25, 2019

Thank you @Koc.

@fabpot fabpot mentioned this pull request Jul 25, 2019
Closed
@fabpot fabpot merged commit eda4f01 into symfony:4.4 Jul 25, 2019
fabpot added a commit that referenced this pull request Jul 25, 2019
@fabpot
feature #32609 [Mailer][DX][RFC] Rename mailer bridge transport class…
fc14cb7 
…es (Koc)

This PR was squashed before being merged into the 4.4 branch (closes #32609).

Discussion
----------

[Mailer][DX][RFC] Rename mailer bridge transport classes

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yno
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

During working on #31946 I realized how painful to work with multiple classes which has same name. [Nice article](https://www.tomasvotruba.cz/blog/2019/05/02/alias-as-a-code-smell/) by @TomasVotruba with explanation of problems  with such approach.
~Built on top of #32608 , so only [2nd commit](bbf7e99) is actual.~

Also I've changed namespaces to make bridge structure much simpler and be linear. All classes located on same level now. See how [bridge](https://github.com/symfony/symfony/tree/bbf7e99e89c70fab372929827ae509b41280ce40/src/Symfony/Component/Mailer/Bridge/Amazon) looks like now.

Now in RFC state to get approve for such king of changes and update all other bridges.

Commits
-------

eda4f01 [Mailer][DX][RFC] Rename mailer bridge transport classes
@fabpot fabpot added the Mailer label Jul 25, 2019
@Koc Koc deleted the rename-mailer-bridge-classes branch July 25, 2019 11:09
@Koc Koc restored the rename-mailer-bridge-classes branch July 25, 2019 11:09
@Koc Koc deleted the rename-mailer-bridge-classes branch July 25, 2019 11:09
@Koc Koc restored the rename-mailer-bridge-classes branch July 25, 2019 11:09
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
BC Break DX DX = Developer eXperience (anything that improves the experience of using Symfony) Mailer RFC RFC = Request For Comments (proposals about features that you want to be discussed) Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
4 participants
@Koc @fabpot @nicolas-grekas @carsonbot