Refactor contact module #8420

schmengler · 2017-02-04T20:15:47Z

This PR contains:

removal of Zend Framework 1 dependencies
usage of result factories in controller
show meaningful error messages to user if possible
separated sendEmail and validatedParams methods for easier rewriting

I intended to move it out of the controller altogether but it seems to be against what the team had in mind with the abstract controller

Why?

In Magento 1 the contact module was a popular one to take as an example for how a simple module should work. Let's keep it aligned with current best practices
Having coding validation and email sending all in the execute method makes extending it hard (as stated above, I'd move it out of the controller but did not want to take the PR too far)

Reference: https://hackernoon.com/the-100-correct-way-to-validate-email-addresses-7c4818f24643

…ct result factory

…cted I intended to move it out of the controller altogether but it seems to be against what the team had in mind with the abstract controller

dmanners · 2017-02-04T20:37:25Z

I think I love you. Death to Zend!

fooman · 2017-02-04T21:42:47Z

app/code/Magento/Contact/Controller/Index/Post.php

+        if (trim($request->getParam('comment')) === '') {
+            throw new LocalizedException(__('Comment is missing'));
+        }
+        if (false === \strpos($request->getParam('email'), '@')) {


You are losing quite a bit of functionality here vs Zend_Validate_EmailAddress. Maybe the better approach would be to switch to the ZF2 component https://github.com/zendframework/zend-validator/blob/master/src/EmailAddress.php#L492

I did that on purpose because I don't believe in email address validation via regular expressions (relevant: https://hackernoon.com/the-100-correct-way-to-validate-email-addresses-7c4818f24643). If there's a decision to use the MX check, the ZF2 email validator would be useful again.

Great article - you got me convinced.

Why are we not using php's native filter_var function? Surely this would be better as:

if (!filter_var($request->getParam('email'), FILTER_VALIDATE_EMAIL)) { throw new LocalizedException(__('Invalid email address')); }

@joe-vortex same reason that I dropped Zend_Validate_EmailAddress. The top comment on http://php.net/manual/en/function.filter-var.php already gives an example

PieterCappelle · 2017-02-05T18:55:27Z

Great PR!

vrann · 2017-02-06T22:53:01Z

@schmengler thank you for contribution, the perfect example of good pull request!
Let me come back on this if there will be any additional questions upon verification.

vrann

Added few comments based on code review and static tests

vrann · 2017-02-07T22:17:57Z

app/code/Magento/Contact/Controller/Index/Post.php

+            ->setTemplateOptions(
+                [
+                    'area' => \Magento\Backend\App\Area\FrontNameResolver::AREA_CODE,
+                    'store' => \Magento\Store\Model\Store::DEFAULT_STORE_ID,


our static test failing with this message:

Module Magento\Contact has undeclared dependencies: hard [Magento\Backend]

do we really need the dependency on Backend here? If yes, please add the dependency to the composer.json otherwise.

This is the same code as before, just moved to a new method, but good that the static tests catched it :) I'll check if we can get rid of the dependency!

yeah, noticed that, though the fact you touched that classed triggered static test failure :) I'd appreciate if you ensure that the dependency is needed and add it to the composer.

vrann · 2017-02-07T22:38:42Z

app/code/Magento/Contact/Controller/Index/Post.php

+     * @param array $post Post data from contact form
+     * @return void
+     */
+    protected function sendEmail($post)


Magento discourages the introduction of any new "protected" properties or methods. The reason is that we are trying to get rid of inheritance-based APIS and promote composition ver inheritance. Only public or private modifiers should be used.

Please change this to "private"

That's good to hear. Would you mind if I removed the abstract base controller then and moved the email sending out of the controller? I hesitated because it looked like inheritance was intended here and extensions might already use it (although that's improbable)

Abstract base controllers (and similar framework pieces such as Blocks, Models, etc) still remain places where legacy inheritance-based APIs haven't been refactored yet. I would suggest keeping abstract base controller for consistency with other controllers (and you are right, for backward compatibility)

However the policy for the new methods/properties to be public or private.

Just to be clear, I was referring to this one, not the framework base controller: https://github.com/magento/magento2/blob/develop/app/code/Magento/Contact/Controller/Index.php

vrann · 2017-02-07T22:40:16Z

app/code/Magento/Contact/Controller/Index/Post.php

-        if (!$post) {
-            $this->_redirect('*/*/');
-            return;
+        if (! $this->isPostRequest()) {


extra space here after "!"

vrann · 2017-02-10T17:34:11Z

@schmengler let me know do you have time to work on requested changes. If you don't at the moment, I'll apply fixes and get it merged.

schmengler · 2017-02-13T07:49:49Z

Thank you! I'll try to get it done this week and will let you know if it does not work out.

…er protected" This reverts commit c8fbb70.

while constants should be preferred over magic strings, the dependency to FrontNameResolver seems arbitrary, just because it happens to have a constant for 'adminhtml' defined.

…contacts module Additionally, the tests have been refactored for current coding standards and PHPUnit forward compatibility

original constants and methods are still in place for backwards compatibility

Conflicts: app/code/Magento/Contact/Test/Unit/Controller/Index/IndexTest.php

… contract

schmengler · 2017-02-16T23:27:19Z

I made the changes, and some more, decided to extract the mail sending functionality from the controller eventually.

I also introduced two API interfaces:

\Magento\Contact\Api\ConfigInterface allows reuse of the contact form with different email template, sender and recipient
\Magento\Contact\Api\MailInterface allows reuse of email sending in different controllers

vrann

The new interfaces do not really belong to Api namespace because they are not service contracts and are not intended to be exposed as Web API. Please move them to the Models namespace and add @api annotation.

There is also one more code style fix with unnecessary space.

Thanks!

vrann · 2017-02-17T19:20:39Z

app/code/Magento/Contact/Api/MailInterface.php

+     * Send email from contact form
+     *
+     * @param string $replyTo Reply-to email address
+     * @param array $variables Email template variables


Service Contracts does not allow usage of "array" type, it should be more specific, i.e. "String[]"

but email template variables can be anything, from strings and integers to objects. How would you deal with it? mixed[] is as unspecific as array, so the only thing I can come up with is to hide the unspecifiy behind a DataObject.

I now understand that these rules only apply to service contracts in the sense of interfaces that can be exposed as web API, so I assume that I can leave the signature as it is, after moving the interfaces out of /Api/

vrann · 2017-02-17T19:21:25Z

app/code/Magento/Contact/Api/ConfigInterface.php

+    /**
+     * Return email recipient address
+     *
+     * @return mixed


Service Contracts does not allow usage of "mixed" type

good catch, should have been string anyway

vrann · 2017-02-17T19:24:53Z

app/code/Magento/Contact/Api/ConfigInterface.php

+/**
+ * Contact module configuration
+ */
+interface ConfigInterface


This interface is neither Service Interface (does not describe operations on data) nor Data Interface (it does not consist only of getters and setters for the fields). Also, it does not looks like it makes sense to expose it as Web API.

Let's move these two interfaces out of Api namespace to the Model. Please add @api annotation to the interface to indicate that this is backward compatible module's interface.

Thank you for the review. I did not intend to expose the interfaces to the web api, they were only meant as service contracts. I was not aware of the convention to distinguish between Api namespace and @api annotation, will change it as suggested

PieterCappelle · 2017-02-17T19:34:55Z

Some extra ideas:

A good next thing would be to send 2 e-mails. One to the admin and one to the submitter with a thank you message.
1b) nl2br from comment message in email
Google Analytics - conversion integration
History of contacts in admin, possibility to export to CSV, ...
Enable phone field setting

justmy2cents

- Merge Pull Request #8434 from Crossmotion/magento2:patch-1

schmengler · 2017-02-19T11:34:01Z

@PieterCappelle this is a bit out of scope for the refactoring. I'm trying not to add new features right now, But that's good material for a follow-up PR!

okorshenko · 2017-02-25T01:25:43Z

@schmengler Thank you for your contribution and this perfect PR!

@kassner

* MAGETWO-64607: Implement custom provider to collect store and system configuration * MAGETWO-64608: Configure store and website reports for data collection * MAGETWO-64612: Implement config for Analytics cron setting * Create wishlist.js * Delete wishlist.js * Update requirejs-config.js * MAGETWO-64607: Implement custom provider to collect store and system configuration * MTO-146: [Test] Create offline Order from "Edit Customer" Admin page - Defects fixed * MTO-146: [Test] Create offline Order from "Edit Customer" Admin page - Defects fixed * MAGETWO-64629: Add plugin for base url change small refactoring * MAGETWO-64630: Create cronjob that executes update command * MAGETWO-64629: Add plugin for base url change review fixes * MTO-142: [Test] Captcha on front login - Defects fixed. * MTO-142: [Test] Captcha on front login - Defects fixed. * MAGETWO-64630: Create cronjob that executes update command * MAGETWO-63406: [Github] Paypal Payment Order Transaction ID Link will result to 404 not found - Fixed returned link - Fixed unit tests * Update Save.php If quantity is changed using mass action for attribute changes, but no stock item exists for the product, saving the newly created stockItem in StockItemRepository->save($stockItem) returns without saving the stockItem, since $stockItem->getProductId() is null, due to it being added to the array with key '0' (see line 165). To fix this, the array key has been changed to "product_id", such that StockItemRepository successfully creates the stockItem. Also, in my opinion, StockItemRepository->save() should not just return without notice, especially as it already encapsulates the functionality in a try/catch block. Throwing an exception would actually tell the user that something went wrong. * MAGETWO-64630: Create cronjob that executes update command * MAGETWO-64224: Remove usages of AttributeCache from customer module - fixing annotation * MAGETWO-64224: Remove usages of AttributeCache from customer module - making inheritdoc lowercase * MTO-146: [Test] Create offline Order from "Edit Customer" Admin page - Defects fixed * MAGETWO-64612: Implement config for Analytics cron setting * MAGETWO-64629: Add plugin for base url change add secure url check * MAGETWO-64629: Add plugin for base url change add secure url check test * MAGETWO-64802: Implement corrupted data reporting for fieldDataConverter * MAGETWO-64505: [Integration] Advanced Analytics subscription create API request add data provider * MAGETWO-64505: [Integration] Advanced Analytics subscription create API request * MTO-111: [Test] Update Category if Use Category Flat (Cron is ON, "Update on Save" Mode) - Defects fixed. * MTO-113: [Test] Captcha on admin login - Defects fixed * MAGETWO-64625: Implement API for file downloading by MA allow only secure connections for analytics * MAGETWO-53457: [COMMUNITY] [FEEDBACK] Improve error reporting for failed order placement (checkout) magento#4682 updated unit tests * MTO-142: [Test] Captcha on front login - Defects fixed. * MTO-113: [Test] Captcha on admin login - Defects fixed * MTO-146: [Test] Create offline Order from "Edit Customer" Admin page - Defects fixed * MAGETWO-64630: Create cronjob that executes update command * MAGETWO-65047 [GitHub][PR] Refactor contact module magento#8420 - fix code style issues * MAGETWO-65047 [GitHub][PR] Refactor contact module magento#8420 - fix integrity tests issues * MAGETWO-65047 [GitHub][PR] Refactor contact module magento#8420 - fix integration tests * MAGETWO-64902: [GitHub][PR] bug magento#8277 fixing bug with https downloading. magento#8278 * MAGETWO-64775: [GitHub] [PR] Make "is_required" and "is_visible" properties of telephone, company and fax attributes of addresses configurable magento#8519 - fix unit and integration tests * MTO-146: [Test] Create offline Order from "Edit Customer" Admin page - Defects fixed * MAGETWO-64802: Implement corrupted data reporting for fieldDataConverter * MAGETWO-64625: Implement API for file downloading by MA add base64 encoding for transferring binary vector * MTO-146: [Test] Create offline Order from "Edit Customer" Admin page - Defects fixed * MAGETWO-64613: Implement service to enrypt collected data * MAGETWO-64775: [GitHub] [PR] Make "is_required" and "is_visible" properties of telephone, company and fax attributes of addresses configurable magento#8519 - fix unit tests * MAGETWO-64613: Implement service to enrypt collected data * MAGETWO-64609: Extend modules report for data colection * MAGETWO-64802: Implement corrupted data reporting for fieldDataConverter * MAGETWO-64802: Implement corrupted data reporting for fieldDataConverter - covered skipping already converted data with integration test * MAGETWO-64608: Configure store and website reports for data collection * MAGETWO-64613: Implement service to enrypt collected data * MAGETWO-64505: [Integration] Advanced Analytics subscription create API request -- fix test * MAGETWO-62322: Performance Profile Generator * MAGETWO-62322: Performance Profile Generator * Add correct return type in order service The method return $this. I corrected the doc-block. * MAGETWO-64625: Implement API for file downloading by MA fix tests * MAGETWO-62549: Collect Data by cron and encrypt file -- fix code style * MAGETWO-64625: Implement API for file downloading by MA allow only secure connections for analytics * MAGETWO-62549: Collect Data by cron and encrypt file -- fix code style * MAGETWO-64802: Implement corrupted data reporting for fieldDataConverter * MTO-146: [Test] Create offline Order from "Edit Customer" Admin page - Issue added to variation * MTO-146: [Test] Create offline Order from "Edit Customer" Admin page - Defects fixed * MAGETWO-64626: Web API test for file export edit test for run on http * MAGETWO-65006: [GitHub][PR] Keep transparency when resizing images magento#7307 - fix is created based on public pull request magento#7307 by @kassner - closing magento#7307 * MAGETWO-51176: ST compiler supplies ObjectManagerInterface to direct third party dependencies * Fixed Doc Block for the dispatch method of the Rest Controller - provided more information on how dispatch method handles the Request * remove unused ComponentRegistrar instance from being passed to Populator::populateMappings * Fixed return type of OrderRepository::getList * MAGETWO-64802: Implement corrupted data reporting for fieldDataConverter - custom converters coverage * MAGETWO-64626: Web API test for file export edit test for run on http * MTO-146: [Test] Create offline Order from "Edit Customer" Admin page - Defects fixed * MAGETWO-64626: Web API test for file export edit test for run on http * MAGETWO-64626: Web API test for file export allow only secure connections for analytics * MTO-111: [Test] Update Category if Use Category Flat (Cron is ON, "Update on Save" Mode) - Duplicated new line removed. * MAGETWO-64626: Web API test for file export edit test for run on http * MTO-146: [Test] Create offline Order from "Edit Customer" Admin page - Defects fixed * MAGETWO-64802: Implement corrupted data reporting for fieldDataConverter - custom converters - fixed integration test and removed dependency * MTO-142: [Test] Captcha on front login - Defects fixed. * MAGETWO-64796: Added fixture for Customer module with assigned website * MAGETWO-64601: [Integration] report page API request for Advanced analytics * MAGETWO-61095: When attempting to place a reorder after a product is disabled, product still gets added to the cart * MAGETWO-61095: When attempting to place a reorder after a product is disabled, product still gets added to the cart * MAGETWO-61095: When attempting to place a reorder after a product is disabled, product still gets added to the cart * MAGETWO-61095: When attempting to place a reorder after a product is disabled, product still gets added to the cart * MAGETWO-61095: When attempting to place a reorder after a product is disabled, product still gets added to the cart * MAGETWO-61095: When attempting to place a reorder after a product is disabled, product still gets added to the cart * MAGETWO-61095: When attempting to place a reorder after a product is disabled, product still gets added to the cart * MAGETWO-61095: When attempting to place a reorder after a product is disabled, product still gets added to the cart * MAGETWO-61095: When attempting to place a reorder after a product is disabled, product still gets added to the cart * MAGETWO-61095: When attempting to place a reorder after a product is disabled, product still gets added to the cart * MAGETWO-64515: change comment in file due to PR risk * MAGETWO-64515: change comment in file due to PR risk * MAGETWO-62168: Improve field data converter logging and error reporting * MAGETWO-62168: Improve field data converter logging and error reporting * MAGETWO-62168: Improve field data converter logging and error reporting * MAGETWO-62168: Improve field data converter logging and error reporting * MAGETWO-62168: Improve field data converter logging and error reporting * MAGETWO-62168: Improve field data converter logging and error reporting

schmengler added 6 commits February 4, 2017 13:32

Remove Zend_Validate calls

47a4681

Replace Zend_Validate_EmailAddress with 100% correct email validation

e1d47cc

Reference: https://hackernoon.com/the-100-correct-way-to-validate-email-addresses-7c4818f24643

Extract sendEmail method in contact form controller

962c06d

Extract validatedParams method in contact form controller, use redire…

fb902c6

…ct result factory

Show meaningful error message if contact form validation fails

352dcef

Make validation and sending methods in contacts form controller prote…

c8fbb70

…cted I intended to move it out of the controller altogether but it seems to be against what the team had in mind with the abstract controller

fooman reviewed Feb 4, 2017

View reviewed changes

schmengler added 3 commits February 4, 2017 22:33

Add docblock comments

36261ea

Update unit test

ec3fad1

Fix code style violations

0c78857

vrann self-assigned this Feb 6, 2017

vrann added the improvement label Feb 6, 2017

vrann suggested changes Feb 7, 2017

View reviewed changes

okorshenko added this to the February 2017 milestone Feb 11, 2017

schmengler added 6 commits February 16, 2017 20:34

Revert "Make validation and sending methods in contacts form controll…

b4f68b0

…er protected" This reverts commit c8fbb70.

Remove unnecessary dependency

5854390

while constants should be preferred over magic strings, the dependency to FrontNameResolver seems arbitrary, just because it happens to have a constant for 'adminhtml' defined.

Moved down unnecessary dependencies from abstract base controller in …

854f16d

…contacts module Additionally, the tests have been refactored for current coding standards and PHPUnit forward compatibility

Move configuration access to config model / API interface

22004d7

original constants and methods are still in place for backwards compatibility

Merge remote-tracking branch 'origin/develop' into contact-refactor

cdf8725

Conflicts: app/code/Magento/Contact/Test/Unit/Controller/Index/IndexTest.php

Extract mail sending from contacts controller, introduce mail service…

28d939f

… contract

schmengler added 2 commits February 16, 2017 23:31

Add docblock comments and empty lines

7a01613

Add missing preference

0261088

vrann suggested changes Feb 17, 2017

View reviewed changes

mmansoor-magento pushed a commit that referenced this pull request Feb 18, 2017

MAGETWO-64250: Merge Pull Requests magento/magento2 #8434, #8417, #8420

459a114

- Merge Pull Request #8434 from Crossmotion/magento2:patch-1

schmengler added 2 commits February 19, 2017 12:47

Move contact form interfaces from Api to Model namespace

a5c69d0

Make method signature of Mail::send() more precise

f5ac9f7

vrann approved these changes Feb 19, 2017

View reviewed changes

orlangur mentioned this pull request Feb 21, 2017

makes $layerResolver protected in Category view #8641

Closed

vrann added the Progress: accept label Feb 23, 2017

mmansoor-magento merged commit f5ac9f7 into magento:develop Feb 25, 2017

tkn98 mentioned this pull request Feb 28, 2017

Email Return-Path Setting is not used as Return-Path in Mail Transport #8543

Closed

markpol mentioned this pull request Mar 5, 2017

merge upstream (#2) #8794

Closed

vrann added the Award: complex label Mar 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor contact module #8420

Refactor contact module #8420

schmengler commented Feb 4, 2017 •

edited by okorshenko

Loading

dmanners commented Feb 4, 2017

fooman Feb 4, 2017

schmengler Feb 4, 2017

fooman Feb 4, 2017

joe-vortex Apr 26, 2018 •

edited

Loading

schmengler May 2, 2018

PieterCappelle commented Feb 5, 2017

vrann commented Feb 6, 2017

vrann left a comment

vrann Feb 7, 2017

schmengler Feb 7, 2017

vrann Feb 9, 2017

vrann Feb 7, 2017

schmengler Feb 7, 2017

vrann Feb 9, 2017

schmengler Feb 13, 2017

vrann Feb 7, 2017

vrann commented Feb 10, 2017

schmengler commented Feb 13, 2017

schmengler commented Feb 16, 2017

vrann left a comment

vrann Feb 17, 2017

schmengler Feb 19, 2017

schmengler Feb 19, 2017

vrann Feb 17, 2017

schmengler Feb 19, 2017

vrann Feb 17, 2017

schmengler Feb 19, 2017

PieterCappelle commented Feb 17, 2017

schmengler commented Feb 19, 2017

okorshenko commented Feb 25, 2017

Refactor contact module #8420

Refactor contact module #8420

Conversation

schmengler commented Feb 4, 2017 • edited by okorshenko Loading

dmanners commented Feb 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joe-vortex Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PieterCappelle commented Feb 5, 2017

vrann commented Feb 6, 2017

vrann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrann commented Feb 10, 2017

schmengler commented Feb 13, 2017

schmengler commented Feb 16, 2017

vrann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PieterCappelle commented Feb 17, 2017

schmengler commented Feb 19, 2017

okorshenko commented Feb 25, 2017

schmengler commented Feb 4, 2017 •

edited by okorshenko

Loading

joe-vortex Apr 26, 2018 •

edited

Loading