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.

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

Add logging to contact us form #9343

Merged
merged 7 commits into from
Apr 30, 2017
Merged

Add logging to contact us form #9343

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
127a134
Add logging to contact us form
Apr 21, 2017
e25dbb2
Update language presented to user
Apr 21, 2017
b69ee25
Update the constructor per the backwards compat policy
Apr 21, 2017
cd0a6e1
Also adjust the data persistor class per code review
Apr 21, 2017
4fe72fb
Fixing line lengths per static analysis
Apr 21, 2017
0be49aa
Undoing optional constructor param per code review
Apr 21, 2017
060e2e8
Remove extra logging per code review
Apr 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions app/code/Magento/Contact/Controller/Index/Post.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Magento\Framework\Controller\Result\Redirect;
use Magento\Framework\Exception\LocalizedException;
use Magento\Framework\HTTP\PhpEnvironment\Request;
use Psr\Log\LoggerInterface;

class Post extends \Magento\Contact\Controller\Index
{
Expand All @@ -32,22 +33,30 @@ class Post extends \Magento\Contact\Controller\Index
*/
private $mail;

/**
* @var LoggerInterface
*/
private $logger;

/**
* @param Context $context
* @param ConfigInterface $contactsConfig
* @param MailInterface $mail
* @param DataPersistorInterface $dataPersistor
* @param LoggerInterface $logger
*/
public function __construct(
Context $context,
ConfigInterface $contactsConfig,
MailInterface $mail,
DataPersistorInterface $dataPersistor
DataPersistorInterface $dataPersistor,
Copy link
Contributor

Choose a reason for hiding this comment

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

current parameter should be optional as well.

$dataPersistor = null

Copy link
Author

Choose a reason for hiding this comment

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

What is the reasoning behind this?

Copy link
Author

Choose a reason for hiding this comment

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

Addressed. I'm concerned about the future look of M2 if these kind of changes continue though. Is the plan just to have optionally parameterized constructors on every class with direct calls to object manager?

Copy link
Contributor

Choose a reason for hiding this comment

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

we have next requirements dictated by Backward Compatibility policy:
Add the new optional parameter to the constructor at the end of arguments list.
In the constructor body, if new dependency is not provided (value of introduced argument is null) fetch the dependency using Magento\Framework\App\ObjectionManager::getInstance(). 

class ExistingClass
{
    /** @var \New\Dependency\Interface */
    private $newDependency;
  
    public function __construct(
        \Old\Dependency\Intreface $oldDependency,
        $oldRequiredConstructorParameter,
        $oldOptinalConstructorParameter = null,
        \New\Dependency\Interface $newDependency = null
    ) {
        ...
        $this->newDependency = $newDependency ?: \Magento\Framework\App\ObjectManager::getInstance()->get(\New\Dependency\Interface::class);
    }
     
    public function existingFunction()
    {
        // Existing functionality
        ...
        ...
        
        // Use $this->newDependency wherever the new dependency is needed
        ...
        ...
    }
}
   
// Sample unit test code snippet follows
class ExistingClassTest extends \PHPUnit_Framework_TestCase
{
    private $existingClassObject;
   
    protected function setUp()
    {
        ...
        // Create dependency mcoks with $this->getMock() or $this->getMockBuilder()
        $newDependencyMock = $this->getMock(\New\Dependency\Interface::class, [], [], '', false);      
          
        $objectManager = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this);
        $this->existingClassObject = $objectManager->getObject(
            ExistingClass::class,
            [
                'oldDependency' => $oldDependencyMock,
                'oldRequiredConstructorParameter' => 'foo',
                'oldOptinalConstructorParameter' => 'bar',
                'newDependency' => $newDependencyMock, 
            ]
        );
    }
   
    public function testExistingFunction()
    {
        ...
        ...
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@JamesonNetworks oh sorry, my fault
I didn't notice that this parameter has already been there before.
DataPersistorInterface $dataPersistor

Sorry, for misleading you.
I was wrong. No need to make it optional.

Copy link
Author

Choose a reason for hiding this comment

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

rgr, no worries, corrected and pushed

Copy link
Author

Choose a reason for hiding this comment

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

I'm not confused about what you are doing, I'm confused as to why. It seems to me you would just mark the PRs that change the constructors and tag them to be released in releases that have the potential to break compatibility. Instead, we're building in tech debt here and expecting a maintainer to come in behind and remove these problems at the time of releases

Copy link
Contributor

Choose a reason for hiding this comment

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

adding an optional parameter to the constructor doesn't bring Backward Incompatible changes. Because code will work according to its contract nevertheless dependency was passed or not (in this case we will instantiate one with ObjectManager::getInstance static call)

LoggerInterface $logger
Copy link
Contributor

Choose a reason for hiding this comment

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

Due to backwards compatibility policy we can not add a new required argument to the constructor.
You may use the injection with the optional parameter, like this:

public function __construct(\NewDependencyInterface $newDependency = null) {
    $this->newDependency = $newDependency ?: \Magento\Framework\App\ObjectManager::getInstance()->get(\NewDependencyInterface::class);
}

Copy link
Author

Choose a reason for hiding this comment

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

Instantiate the object directly with object manager? My understanding is that is a big no no

Copy link
Author

Choose a reason for hiding this comment

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

Regardless, updated. Would like clarification on when it is ok to use object manager. Maybe only in core code?

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, using ObjectManager directly is indeed a "big no no". However, some existing extension or customization may possibly extend this class, thus, by changing the constructor signature you would break it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Such direct usages of the ObjectManager should be refactored eventually, but that may only happen we module version is bumped.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, thanks for the clarification!

) {
parent::__construct($context, $contactsConfig);
$this->context = $context;
$this->mail = $mail;
$this->dataPersistor = $dataPersistor;
$this->logger = $logger;
}

/**
Expand All @@ -70,8 +79,10 @@ public function execute()
$this->messageManager->addErrorMessage($e->getMessage());
$this->getDataPersistor()->set('contact_us', $this->getRequest()->getParams());
} catch (\Exception $e) {
$this->logger->addError('An error occurred processing a posted message to contact us in Controller\\Index\\Post: ' . $e->getMessage());
$this->logger->critical($e);
$this->messageManager->addErrorMessage(
__('We can\'t process your request right now. Sorry, that\'s all we know.')
__('An error occurred while processing your form. Please try again later.')
);
$this->getDataPersistor()->set('contact_us', $this->getRequest()->getParams());
}
Expand Down