From f2e0dc7d2f32c4100f147ef0f75d420f33d5a9bc Mon Sep 17 00:00:00 2001 From: wxa Date: Wed, 24 Apr 2019 16:50:10 +0300 Subject: [PATCH] Refactor URL helper to DI and move its test to unit [MAILPOET-2009] --- lib/Config/Changelog.php | 11 ++++-- lib/Config/Hooks.php | 10 ++++- lib/DI/ContainerConfigurator.php | 5 +++ lib/Subscription/Form.php | 12 ++++-- lib/Subscription/Manage.php | 30 +++++++++----- lib/Subscription/Pages.php | 6 ++- lib/Util/Url.php | 39 +++++++++++-------- .../ConfirmationEmailMailerTest.php | 1 + tests/integration/Subscription/FormTest.php | 30 +++++++------- tests/integration/Util/UrlTest.php | 12 ------ tests/unit/Util/UrlTest.php | 18 +++++++++ 11 files changed, 111 insertions(+), 63 deletions(-) delete mode 100644 tests/integration/Util/UrlTest.php create mode 100644 tests/unit/Util/UrlTest.php diff --git a/lib/Config/Changelog.php b/lib/Config/Changelog.php index 006d6063b1..a1eae7f38e 100644 --- a/lib/Config/Changelog.php +++ b/lib/Config/Changelog.php @@ -17,14 +17,19 @@ class Changelog { /** @var Helper */ private $wooCommerceHelper; + /** @var Url */ + private $url_helper; + function __construct( SettingsController $settings, WPFunctions $wp, - Helper $wooCommerceHelper + Helper $wooCommerceHelper, + Url $url_helper ) { $this->wooCommerceHelper = $wooCommerceHelper; $this->settings = $settings; $this->wp = $wp; + $this->url_helper = $url_helper; } function init() { @@ -101,13 +106,13 @@ class Changelog { && $this->wooCommerceHelper->getOrdersCount() >= 1 && $this->wp->currentUserCan('administrator') ) { - Url::redirectTo($this->wp->adminUrl('admin.php?page=mailpoet-woocommerce-list-import')); + $this->url_helper->redirectTo($this->wp->adminUrl('admin.php?page=mailpoet-woocommerce-list-import')); } } private function terminateWithRedirect($redirect_url) { // save version number $this->settings->set('version', Env::$version); - Url::redirectWithReferer($redirect_url); + $this->url_helper->redirectWithReferer($redirect_url); } } diff --git a/lib/Config/Hooks.php b/lib/Config/Hooks.php index 7cc1f0f331..144705f0d5 100644 --- a/lib/Config/Hooks.php +++ b/lib/Config/Hooks.php @@ -6,6 +6,7 @@ use MailPoet\Settings\SettingsController; use MailPoet\Statistics\Track\WooCommercePurchases; use MailPoet\Subscription\Comment; use MailPoet\Subscription\Form; +use MailPoet\Subscription\Manage; use MailPoet\Subscription\Registration; use MailPoet\Segments\WooCommerce as WooCommerceSegment; use MailPoet\WooCommerce\Subscription as WooCommerceSubscription; @@ -19,6 +20,9 @@ class Hooks { /** @var Comment */ private $subscription_comment; + /** @var Manage */ + private $subscription_manage; + /** @var Registration */ private $subscription_registration; @@ -40,6 +44,7 @@ class Hooks { function __construct( Form $subscription_form, Comment $subscription_comment, + Manage $subscription_manage, Registration $subscription_registration, SettingsController $settings, WPFunctions $wp, @@ -49,6 +54,7 @@ class Hooks { ) { $this->subscription_form = $subscription_form; $this->subscription_comment = $subscription_comment; + $this->subscription_manage = $subscription_manage; $this->subscription_registration = $subscription_registration; $this->settings = $settings; $this->wp = $wp; @@ -138,11 +144,11 @@ class Hooks { // Manage subscription $this->wp->addAction( 'admin_post_mailpoet_subscription_update', - '\MailPoet\Subscription\Manage::onSave' + [$this->subscription_manage, 'onSave'] ); $this->wp->addAction( 'admin_post_nopriv_mailpoet_subscription_update', - '\MailPoet\Subscription\Manage::onSave' + [$this->subscription_manage, 'onSave'] ); // Subscription form diff --git a/lib/DI/ContainerConfigurator.php b/lib/DI/ContainerConfigurator.php index 76f88e2a21..6ebcc75520 100644 --- a/lib/DI/ContainerConfigurator.php +++ b/lib/DI/ContainerConfigurator.php @@ -71,6 +71,8 @@ class ContainerConfigurator implements IContainerConfigurator { $container->autowire(\MailPoet\Cron\Workers\SendingQueue\SendingErrorHandler::class)->setPublic(true); $container->autowire(\MailPoet\Cron\Workers\StatsNotifications\Scheduler::class); $container->autowire(\MailPoet\Cron\CronTrigger::class)->setPublic(true); + // Form + $container->autowire(\MailPoet\Form\Util\FieldNameObfuscator::class)->setPublic(true); // Listing $container->autowire(\MailPoet\Listing\BulkActionController::class)->setPublic(true); $container->autowire(\MailPoet\Listing\BulkActionFactory::class)->setPublic(true); @@ -103,9 +105,12 @@ class ContainerConfigurator implements IContainerConfigurator { // Subscription $container->autowire(\MailPoet\Subscription\Comment::class)->setPublic(true); $container->autowire(\MailPoet\Subscription\Form::class)->setPublic(true); + $container->autowire(\MailPoet\Subscription\Manage::class)->setPublic(true); $container->autowire(\MailPoet\Subscription\Registration::class)->setPublic(true); // Newsletter $container->autowire(\MailPoet\Newsletter\AutomatedLatestContent::class)->setPublic(true); + // Util + $container->autowire(\MailPoet\Util\Url::class)->setPublic(true); // WooCommerce $container->autowire(\MailPoet\WooCommerce\Helper::class)->setPublic(true); $container->autowire(\MailPoet\WooCommerce\Subscription::class)->setPublic(true); diff --git a/lib/Subscription/Form.php b/lib/Subscription/Form.php index 5aa4368990..9a965c3760 100644 --- a/lib/Subscription/Form.php +++ b/lib/Subscription/Form.php @@ -11,8 +11,12 @@ class Form { /** @var API */ private $api; - function __construct(API $api) { + /** @var UrlHelper */ + private $url_helper; + + function __construct(API $api, UrlHelper $url_helper) { $this->api = $api; + $this->url_helper = $url_helper; } function onSubmit($request_data = false) { @@ -21,7 +25,7 @@ class Form { $form_id = (!empty($request_data['data']['form_id'])) ? (int)$request_data['data']['form_id'] : false; $response = $this->api->processRoute(); if ($response->status !== APIResponse::STATUS_OK) { - return UrlHelper::redirectBack( + return $this->url_helper->redirectBack( array( 'mailpoet_error' => ($form_id) ? $form_id : true, 'mailpoet_success' => null @@ -29,8 +33,8 @@ class Form { ); } else { return (isset($response->meta['redirect_url'])) ? - UrlHelper::redirectTo($response->meta['redirect_url']) : - UrlHelper::redirectBack( + $this->url_helper->redirectTo($response->meta['redirect_url']) : + $this->url_helper->redirectBack( array( 'mailpoet_success' => $form_id, 'mailpoet_error' => null diff --git a/lib/Subscription/Manage.php b/lib/Subscription/Manage.php index 710ae48895..876520fb65 100644 --- a/lib/Subscription/Manage.php +++ b/lib/Subscription/Manage.php @@ -5,33 +5,43 @@ namespace MailPoet\Subscription; use MailPoet\Form\Util\FieldNameObfuscator; use MailPoet\Models\CustomField; use MailPoet\Models\Subscriber; -use MailPoet\Util\Url; +use MailPoet\Util\Url as UrlHelper; class Manage { - static function onSave() { + /** @var UrlHelper */ + private $url_helper; + + /** @var FieldNameObfuscator */ + private $field_name_obfuscator; + + function __construct(UrlHelper $url_helper, FieldNameObfuscator $field_name_obfuscator) { + $this->url_helper = $url_helper; + $this->field_name_obfuscator = $field_name_obfuscator; + } + + function onSave() { $action = (isset($_POST['action']) ? $_POST['action'] : null); $token = (isset($_POST['token']) ? $_POST['token'] : null); if ($action !== 'mailpoet_subscription_update' || empty($_POST['data'])) { - Url::redirectBack(); + $this->url_helper->redirectBack(); } $subscriber_data = $_POST['data']; - $obfuscator = new FieldNameObfuscator(); - $subscriber_data = $obfuscator->deobfuscateFormPayload($subscriber_data); + $subscriber_data = $this->field_name_obfuscator->deobfuscateFormPayload($subscriber_data); if (!empty($subscriber_data['email']) && Subscriber::verifyToken($subscriber_data['email'], $token)) { if ($subscriber_data['email'] !== Pages::DEMO_EMAIL) { - $subscriber = Subscriber::createOrUpdate(static::filterOutEmptyMandatoryFields($subscriber_data)); + $subscriber = Subscriber::createOrUpdate($this->filterOutEmptyMandatoryFields($subscriber_data)); $subscriber->getErrors(); } } - Url::redirectBack(); + $this->url_helper->redirectBack(); } - private static function filterOutEmptyMandatoryFields(array $subscriber_data) { - $mandatory = self::getMandatory(); + private function filterOutEmptyMandatoryFields(array $subscriber_data) { + $mandatory = $this->getMandatory(); foreach ($mandatory as $name) { if (strlen(trim($subscriber_data[$name])) === 0) { unset($subscriber_data[$name]); @@ -40,7 +50,7 @@ class Manage { return $subscriber_data; } - private static function getMandatory() { + private function getMandatory() { $mandatory = []; $required_custom_fields = CustomField::findMany(); foreach ($required_custom_fields as $custom_field) { diff --git a/lib/Subscription/Pages.php b/lib/Subscription/Pages.php index 3efd7c7e2d..a31748d691 100644 --- a/lib/Subscription/Pages.php +++ b/lib/Subscription/Pages.php @@ -29,6 +29,9 @@ class Pages { /** @var SettingsController */ private $settings; + /** @var UrlHelper */ + private $url_helper; + function __construct($action = false, $data = array(), $init_shortcodes = false, $init_page_filters = false, $new_subscriber_notification_sender = null) { $this->action = $action; $this->data = $data; @@ -41,6 +44,7 @@ class Pages { $this->new_subscriber_notification_sender = new NewSubscriberNotificationMailer(); } $this->settings = new SettingsController(); + $this->url_helper = new UrlHelper(new WPFunctions()); } private function isPreview() { @@ -395,7 +399,7 @@ class Pages { ' value="mailpoet_subscription_update" />'; $form_html .= ''; $form_html .= ''; + 'value="' . htmlspecialchars($this->url_helper->getCurrentUrl(), ENT_QUOTES) . '" />'; $form_html .= ''; diff --git a/lib/Util/Url.php b/lib/Util/Url.php index da33ab6504..028d199f3c 100644 --- a/lib/Util/Url.php +++ b/lib/Util/Url.php @@ -4,47 +4,54 @@ namespace MailPoet\Util; use MailPoet\WP\Functions as WPFunctions; class Url { - static function getCurrentUrl() { - $home_url = parse_url(home_url()); - $query_args = WPFunctions::get()->addQueryArg(null, null); + /** @var WPFunctions */ + private $wp; - // Remove WPFunctions::get()->homeUrl() path from add_query_arg + function __construct(WPFunctions $wp) { + $this->wp = $wp; + } + + function getCurrentUrl() { + $home_url = parse_url($this->wp->homeUrl()); + $query_args = $this->wp->addQueryArg(null, null); + + // Remove $this->wp->homeUrl() path from add_query_arg if (isset($home_url['path'])) { $query_args = str_replace($home_url['path'], '', $query_args); } - return WPFunctions::get()->homeUrl($query_args); + return $this->wp->homeUrl($query_args); } - static function redirectTo($url = null) { - WPFunctions::get()->wpSafeRedirect($url); + function redirectTo($url = null) { + $this->wp->wpSafeRedirect($url); exit(); } - static function redirectBack($params = array()) { + function redirectBack($params = array()) { // check mailpoet_redirect parameter $referer = (isset($_POST['mailpoet_redirect']) ? $_POST['mailpoet_redirect'] - : WPFunctions::get()->wpGetReferer() + : $this->wp->wpGetReferer() ); // fallback: home_url if (!$referer) { - $referer = WPFunctions::get()->homeUrl(); + $referer = $this->wp->homeUrl(); } // append extra params to url if (!empty($params)) { - $referer = WPFunctions::get()->addQueryArg($params, $referer); + $referer = $this->wp->addQueryArg($params, $referer); } - self::redirectTo($referer); + $this->redirectTo($referer); exit(); } - static function redirectWithReferer($url = null) { - $current_url = self::getCurrentUrl(); - $url = WPFunctions::get()->addQueryArg( + function redirectWithReferer($url = null) { + $current_url = $this->getCurrentUrl(); + $url = $this->wp->addQueryArg( array( 'mailpoet_redirect' => urlencode($current_url) ), @@ -52,7 +59,7 @@ class Url { ); if ($url !== $current_url) { - self::redirectTo($url); + $this->redirectTo($url); } exit(); } diff --git a/tests/integration/Subscribers/ConfirmationEmailMailerTest.php b/tests/integration/Subscribers/ConfirmationEmailMailerTest.php index 4b09897065..36328aec59 100644 --- a/tests/integration/Subscribers/ConfirmationEmailMailerTest.php +++ b/tests/integration/Subscribers/ConfirmationEmailMailerTest.php @@ -116,6 +116,7 @@ class ConfirmationEmailMailerTest extends \MailPoetTest { } function _after() { + Mock::clean(); \ORM::raw_execute('TRUNCATE ' . Subscriber::$_table); \ORM::raw_execute('TRUNCATE ' . Segment::$_table); \ORM::raw_execute('TRUNCATE ' . SubscriberSegment::$_table); diff --git a/tests/integration/Subscription/FormTest.php b/tests/integration/Subscription/FormTest.php index 08c821003e..a30c0f50b5 100644 --- a/tests/integration/Subscription/FormTest.php +++ b/tests/integration/Subscription/FormTest.php @@ -1,7 +1,8 @@ settings->set('signup_confirmation.enabled', false); - $this->form_controller = ContainerWrapper::getInstance()->get(Form::class); } function testItSubscribesAndRedirectsBackWithSuccessResponse() { - $mock = Mock::double('MailPoet\Util\Url', [ + $url_helper = Stub::make(UrlHelper::class, [ 'redirectBack' => function($params) { return $params; } - ]); - $result = $this->form_controller->onSubmit($this->request_data); + ], $this); + $form_controller = new Form(ContainerWrapper::getInstance()->get(API::class), $url_helper); + $result = $form_controller->onSubmit($this->request_data); expect(SubscriberModel::findOne($this->testEmail))->notEmpty(); - $mock->verifyInvoked('redirectBack'); expect($result['mailpoet_success'])->equals($this->form->id); expect($result['mailpoet_error'])->null(); } @@ -92,17 +93,17 @@ class FormTest extends \MailPoetTest { $form_settings['success_page'] = $this->post; $form->settings = serialize($form_settings); $form->save(); - $mock = Mock::double('MailPoet\Util\Url', [ + $url_helper = Stub::make(UrlHelper::class, [ 'redirectTo' => function($params) { return $params; }, 'redirectBack' => function($params) { return $params; } - ]); - $result = $this->form_controller->onSubmit($this->request_data); + ], $this); + $form_controller = new Form(ContainerWrapper::getInstance()->get(API::class), $url_helper); + $result = $form_controller->onSubmit($this->request_data); expect(SubscriberModel::findOne($this->testEmail))->notEmpty(); - $mock->verifyInvoked('redirectTo'); expect($result)->regExp('/http.*?sample-post/i'); } @@ -110,20 +111,19 @@ class FormTest extends \MailPoetTest { // clear subscriber email so that subscription fails $request_data = $this->request_data; $request_data['data']['email'] = false; - $mock = Mock::double('MailPoet\Util\Url', [ + $url_helper = Stub::make(UrlHelper::class, [ 'redirectBack' => function($params) { return $params; } - ]); - $result = $this->form_controller->onSubmit($request_data); + ], $this); + $form_controller = new Form(ContainerWrapper::getInstance()->get(API::class), $url_helper); + $result = $form_controller->onSubmit($request_data); expect(SubscriberModel::findMany())->isEmpty(); - $mock->verifyInvoked('redirectBack'); expect($result['mailpoet_error'])->equals($this->form->id); expect($result['mailpoet_success'])->null(); } function _after() { - Mock::clean(); wp_delete_post($this->post); \ORM::raw_execute('TRUNCATE ' . SegmentModel::$_table); \ORM::raw_execute('TRUNCATE ' . FormModel::$_table); diff --git a/tests/integration/Util/UrlTest.php b/tests/integration/Util/UrlTest.php deleted file mode 100644 index e6b12ac688..0000000000 --- a/tests/integration/Util/UrlTest.php +++ /dev/null @@ -1,12 +0,0 @@ -equals($home_url); - } -} \ No newline at end of file diff --git a/tests/unit/Util/UrlTest.php b/tests/unit/Util/UrlTest.php new file mode 100644 index 0000000000..ec6adcab59 --- /dev/null +++ b/tests/unit/Util/UrlTest.php @@ -0,0 +1,18 @@ + $home_url, + 'addQueryArg' => '', + ])); + $current_url = $url_helper->getCurrentUrl(); + expect($current_url)->equals($home_url); + } +} \ No newline at end of file