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

EmailBundle - removing deprecated code #8049

Merged

Conversation

escopecz
Copy link
Sponsor Member

@escopecz escopecz commented Nov 1, 2019

Please be sure you are submitting this against the staging branch.

Q A
Bug fix?
New feature?
Automated tests included?
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs) #7998
BC breaks?
Deprecations?

Description:

Steps to test this PR:

  1. Load up this PR
  2. Test all forms related to emails.
  3. Including email related actions in campaign, form and point actions.

List deprecations along with the new alternative:

In https://docs.google.com/document/d/1lHNK8hHQtvyWlz9OtfAoLupDVb5-QunQjq-HAGlAsS4/edit#

List backwards compatibility breaks:

In https://docs.google.com/document/d/1lHNK8hHQtvyWlz9OtfAoLupDVb5-QunQjq-HAGlAsS4/edit#

@escopecz escopecz changed the title EmailType got rid of MauticFactory and other improvements EmailBundle - removing deprecated code Nov 1, 2019
@npracht npracht added Mautic 3 WIP PR's that are not ready for review and are currently in progress labels Nov 1, 2019
@npracht npracht added this to the 3.0.0 milestone Nov 1, 2019
@escopecz escopecz changed the base branch from staging to m2-to-m3 November 1, 2019 13:04
@escopecz escopecz removed the WIP PR's that are not ready for review and are currently in progress label Nov 1, 2019
@npracht npracht added code-review-needed PR's that require a code review before merging ready-to-test PR's that are ready to test labels Nov 1, 2019
@escopecz escopecz force-pushed the email-bundle-deprecation-refactoring branch 2 times, most recently from 6814832 to ca3356a Compare November 5, 2019 09:58
Copy link

@florentpetitjean florentpetitjean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,

I've tested the followings :

  • Create an email
  • Clone an email
  • Delete an email
  • Open builder
  • Change theme

All is working :)

@npracht npracht removed the ready-to-test PR's that are ready to test label Nov 7, 2019
@escopecz escopecz force-pushed the email-bundle-deprecation-refactoring branch from ca3356a to 7d5a4dd Compare November 26, 2019 08:39
@dongilbert
Copy link
Member

Some conflicts need resolving

@escopecz
Copy link
Sponsor Member Author

escopecz commented Dec 3, 2019

Shouldn't we flip keys and values for ChoiceType type and use 'choices_as_values' option? (actually you showed me this)

Yes, it should. I must have forgot that step here. Thanks for noticing. Ready for second round.

@escopecz escopecz added ready-to-test PR's that are ready to test and removed WIP PR's that are not ready for review and are currently in progress labels Dec 3, 2019
Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\Mautic\EmailBundle\Form\Type\EmailType
Could you please change form type to FQCN on lines 505 and 506 (hidden type)?

@escopecz
Copy link
Sponsor Member Author

escopecz commented Dec 4, 2019

@anton-vlasenko thanks! Fixed.

Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code seems to be fine.
But when I opened an "email view" page (it was a transactional email) and clicked on the "Add A/B test" dropdown option I got the following error:
Screenshot 2019-12-04 at 12 03 25

@anton-vlasenko anton-vlasenko added pending-feedback PR's and issues that are awaiting feedback from the author and removed code-review-needed PR's that require a code review before merging labels Dec 4, 2019
Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry that I don't post all my findings in one post, but I keep finding items that have to be refactored (in my opinion).

  1. A call to setMauticFactory method still present in the \Mautic\EmailBundle\Controller\AjaxController (lines 228, 229)
  2. \Mautic\EmailBundle\Controller\EmailController extends Mautic\CoreBundle\Controller\FormController. FormController is deprecated. EmailController is also using $this->factory, which is also deprecated (MauticFactory).
  3. \Mautic\EmailBundle\Controller\PublicController extends Mautic\CoreBundle\Controller\FormController. FormController is deprecated. PublicController is also using $this->factory, which is also deprecated (MauticFactory).
  4. \Mautic\EmailBundle\Helper\MailHelper still uses MauticFactory ($this->factory).
  5. \Mautic\EmailBundle\Swiftmailer\Transport\AbstractTokenArrayTransport still uses MauticFactory. It also has setMauticFactory method defined, which is deprecated.
  6. \Mautic\EmailBundle\Swiftmailer\Transport\InterfaceTokenTransport is deprecated and should be removed.
  7. \Mautic\EmailBundle\EmailEvents::ON_CAMPAIGN_TRIGGER_ACTION is deprecated and should be removed.
  8. \Mautic\EmailBundle\Swiftmailer\Transport\AbstractTokenArrayTransport::$factory is deprecated (MauticFactory).
  9. \Mautic\EmailBundle\Swiftmailer\Transport\AbstractTokenArrayTransport::setMauticFactory is deprecated.
  10. \Mautic\EmailBundle\Swiftmailer\Transport\MandrillTransport on lines 253 and 289 it says that the BC support @deprecated. I think this has to be refactored.

@anton-vlasenko anton-vlasenko added the code-review-needed PR's that require a code review before merging label Dec 4, 2019
@escopecz
Copy link
Sponsor Member Author

escopecz commented Dec 4, 2019

I will check the VariantType once my laptop is fixed.

We are not dealing with controllers in M3 and there is another issue to refactor the transports so those won't be refactored in this PR.

@anton-vlasenko
Copy link
Contributor

anton-vlasenko commented Dec 4, 2019

there is another issue to refactor the transports so those won't be refactored in this PR.

@escopecz Thank you for explaining to me. I haven't noticed these issues on the board.

I just don't understand how the controllers are going to work if we don't refactor them. But If you say that they should not be refactored now I'm fine with that. I'm just curious what our plan is.
2. I'd like to ask what to do with the \Mautic\EmailBundle\EmailEvents::ON_CAMPAIGN_TRIGGER_ACTION. It's deprecated and it's not in the controller and/or transport.

@escopecz
Copy link
Sponsor Member Author

escopecz commented Dec 5, 2019

@anton-vlasenko I fixed the VariantType and created issue for the campaign batch deprecation #8191

@escopecz escopecz removed the pending-feedback PR's and issues that are awaiting feedback from the author label Dec 5, 2019
Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\Mautic\EmailBundle\Helper\MailHelper still uses MauticFactory.
It's not a controller/transport.
IMO we either have to refactor it in this PR or create an issue for it (I can't find it on the board).

@anton-vlasenko anton-vlasenko added the pending-feedback PR's and issues that are awaiting feedback from the author label Dec 6, 2019
@escopecz
Copy link
Sponsor Member Author

escopecz commented Dec 6, 2019

@anton-vlasenko here is the issue: #8060

Just to be clear what the current goal is. Let's make Mautic work on Symfony 3.4. If there will be time to refactor more we can do that. Removing MauticFactory is secondary goal. Let's focus on the primary goal.

Copy link
Contributor

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how I missed the issue.
The code looks fine.
I've tested it and didn't find any errors.
So I approve this PR.

@anton-vlasenko
Copy link
Contributor

@escopecz Could you please merge it?

@escopecz escopecz merged commit 97d8d52 into mautic:m2-to-m3 Dec 6, 2019
@escopecz escopecz deleted the email-bundle-deprecation-refactoring branch December 6, 2019 13:34
@escopecz escopecz added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed code-review-needed PR's that require a code review before merging pending-feedback PR's and issues that are awaiting feedback from the author ready-to-test PR's that are ready to test labels Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants