Skip to content

Mentioning Symfony Mailer requirement in v2.0.0 #126

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

Merged
merged 2 commits into from
Mar 28, 2021

Conversation

ThomasLandauer
Copy link
Member

@ThomasLandauer ThomasLandauer commented Mar 24, 2021

Follow-up of #125. My plan would be to copy this line to all email-related functions. Questions:

  1. Is ^1.6 right?
  2. The mailer configuration was never documented anywhere. But in v1.6, Swift Mailer is the default anyway, right? So this configuration is only relevant for people who still have v1.6 but use Symfony Mailer? And in this case, the better advice is to upgrade to v2, rather than configuring mailer?

My plan would be to copy this line to all email-related functions. Questions:
1. Is `^1.6` right?
2. The `mailer` configuration was never documented anywhere. But in v1.6, Swift Mailer is the default anyway, right? So this configuration is only relevant for people who still have v1.6 but use Symfony Mailer? And in this case, the better advice is to upgrade to v2, rather than configuring `mailer`?
@TavoNiievez
Copy link
Member

Do you think we can move this to the top of the documentation page ?, I'm not sure if this is the best place to remember something like that.

@ThomasLandauer
Copy link
Member Author

Do you think we can move this to the top of the documentation page ?, I'm not sure if this is the best place to remember something like that.

I thought about that too. My counter-arguments were:

  • It's not important for everybody but just for users of the email functions.
  • If somebody misses it, the fix is quite easy (downgrade), so it's not really an "alert" we need to throw upon everybody.
  • If it's mentioned at the very function which ultimately causes the fail, it probably gets more attention (by the "relevant" people) than if it's included in the "foreword".

But I'm not 100% sure myself ;-) What/where would you suggest?
By "remember" to you mean the users, or you/us (the maintainers)?

@TavoNiievez
Copy link
Member

By "remember" to you mean the users, or you/us (the maintainers)?

I mean the users, not the maintainers.

What/where would you suggest?

After reading your own counterarguments, I still think it's best to just say it once.

@ThomasLandauer
Copy link
Member Author

After reading your own counterarguments, I still think it's best to just say it once.

OK, then some box at the top with the current text. But on each function a link to this box?! (In the end, the link will be almost as long as the entire text... ;-)

@TavoNiievez
Copy link
Member

But on each function a link to this box?!

No, no link.

Having to mention compatibility with specific versions of other packages in the method docblocks is something I don't like at all.

I think a better solution would be to nip the problem in the bud and directly block the installation of version 2.0 if you have swift mailer installed.

As far as I know, composer is smart enough in those scenarios to install the closest lower version that does not include a conflicts directive.

Or I understand that Codeception has a "ConflictsWith" interface to handle these cases.

@Naktibalda What do you think?

@Naktibalda
Copy link
Member

Codeception has ConflictsWithModule interface which is not applicable in this case.

conflicts in composer.json looks like a good solution, but you would have to re-release 2.0.0 with added conflicts clause to get it working properly.
A small number of users who already have 2.0.0 installed and use SwiftMailer won't be happy about it.

@TavoNiievez TavoNiievez linked an issue Mar 28, 2021 that may be closed by this pull request
@ThomasLandauer
Copy link
Member Author

Having to mention compatibility with specific versions of other packages

Which other packages? We're only talking about this module's versions. The current text in this PR is valid "forever" (so no maintenance problem).

But more important: @TavoNiievez I don't think your plan is a good solution. Reasons:

  • Having Swift Mailer installed does not conflict with v2! You just can't use the email related functions ;-)
  • On large projects, moving away from Swift Mailer might be done in steps. If they start migrating the important emails to Symfony Mailer first, they can use v2, and still continue to use Swift Mailer for some not-so-important emails (for which they don't have tests).

So my suggestions are (in that order):

  1. Back to my original plan (i.e. repeat the warning on each function). I mean, there's so many outdated information on this website (which I'm trying to get rid of, step by step), that the worst case scenario (a handful of irrelevant - not outdated! - notices on some functions) doesn't really threaten me ;-)
  2. If you still don't like that, let's go for a caution box on top of the page, with links from each function.
  3. If you don't like that either, just the caution box (no links)
  4. If you don't like that too, just close this PR and do nothing. Sooner or later they'll see the Fail message which explains it anyway ;-)

But this entire question is not important enough for ongoing discussion! Whichever solution it'll gonna be, the time spent discussing it is already longer than the time it'll take to implement it ;-)
So pick one: 1,2,3,4

@TavoNiievez TavoNiievez added this to the 2.0.0 milestone Mar 28, 2021
@TavoNiievez TavoNiievez merged commit bfc25c7 into Codeception:master Mar 28, 2021
@ThomasLandauer ThomasLandauer deleted the patch-4 branch March 28, 2021 15:21
@TavoNiievez
Copy link
Member

@ThomasLandauer Thank you for your patience in this matter.

@ThomasLandauer
Copy link
Member Author

@TavoNiievez Never mind :-)
But I still think that doing a 2.0.1 release is not a good idea! (see reasons above...)

@TavoNiievez TavoNiievez modified the milestones: 2.0.0, 2.0.1 Mar 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Status of Swift Mailer support in v2.0.0?
3 participants