Add logging to contact us form

[JamesonNetworks]

Description

If contact us fails to process, the exception is swallowed

Fixed Issues (if relevant)

I did not open an issue for this problem

Manual testing scenarios

1. Create conditions for the contact us form to fail. I did this by changing the code in the controller to explicitly throw an exception, however, I first experienced this in the Magento Cloud where no useful information is being retained.

Contribution checklist

• [Y] Pull request has a meaningful description of its purpose
• [Y] All commits are accompanied by meaningful commit messages
• [N/A] All new or changed code is covered with unit/integration tests (if applicable)
• [Y] All automated tests passed successfully (all builds on Travis CI are green)

[ishakhsuvarov]

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);
}
     

[JamesonNetworks]

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

[JamesonNetworks]

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

[ishakhsuvarov]

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.

[ishakhsuvarov]

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

[JamesonNetworks]

Makes sense, thanks for the clarification!

[maghamed]

current parameter should be optional as well.

$dataPersistor = null

[JamesonNetworks]

What is the reasoning behind this?

[JamesonNetworks]

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?

[maghamed]

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()
    {
        ...
        ...
    }
}

[maghamed]

@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.

[JamesonNetworks]

rgr, no worries, corrected and pushed

[JamesonNetworks]

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

[maghamed]

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)

[adragus-inviqa]

a) why do we need this one? wouldn't the stack trace by enough?
b) if you're going to do it, why aren't you using __CLASS__?

[JamesonNetworks]

a) I'm not sure what M2 logging best practices are so I was throwing it in the two places I would expect to see it.
b) Should have used __CLASS__ here.

I removed this line since the critical log could be enough? If you have any pointers to docs on best practices for core logging, I would appreciate it.

Thanks!

[JamesonNetworks]

@adragus-inviqa @ishakhsuvarov @maghamed Thanks everyone for the review and input! I appreciate the feedback

[magento-team]

@JamesonNetworks thank you for your contribution. Your Pull Request has been successfully merged