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

fix: Email SMTP may throw Uncaught ErrorException #6184

Merged

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jun 24, 2022

Description
Fixes #3390

Checklist:

  • Securely signed commits
  • [] Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Jun 24, 2022
@kenjis
Copy link
Member Author

kenjis commented Jun 24, 2022

@samsonasik The following refactoring breaks test code.

Method name is not configured

1) tests/system/Email/EmailTest.php:138

    ---------- begin diff ----------
@@ @@

     public function testDestructDoesNotThrowException()
     {
-        $email = $this->getMockBuilder(Email::class)
-            ->disableOriginalConstructor()
-            ->onlyMethods(['sendCommand'])
-            ->getMock();
+        $email = $this->createMock(Email::class);
         $email->method('sendCommand')
-            ->will($this->throwException(new ErrorException('SMTP Error.')));
+            ->willThrowException(new ErrorException('SMTP Error.'));

         // Force resource to be injected into the property
         $SMTPConnect = fopen(__FILE__, 'rb');
    ----------- end diff -----------

Applied rules:
 * GetMockBuilderGetMockToCreateMockRector
 * UseSpecificWillMethodRector

@samsonasik
Copy link
Member

samsonasik commented Jun 24, 2022

@kenjis could you create issue/failing test case on rector-phpunit repo? Should be bug on GetMockBuilderGetMockToCreateMockRector

@kenjis kenjis force-pushed the fix-email-Uncaught-ErrorException branch from 47643f9 to c4022bb Compare June 24, 2022 08:17
@kenjis
Copy link
Member Author

kenjis commented Jun 24, 2022

@samsonasik Created: rectorphp/rector-phpunit#86

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Looks good pending the Rector PR.

@paulbalandan
Copy link
Member

We can use $this->createPartialMock(Email::class, ['sendCommand']); for brevity. I think this is what Rector should have done.

@kenjis
Copy link
Member Author

kenjis commented Jun 24, 2022

We can use $this->createPartialMock(Email::class, ['sendCommand']); for brevity. I think this is what Rector should have done.

createPartialMock() uses setMethods() that is deprecated.
So using createPartialMock() changes the behavior.

@paulbalandan
Copy link
Member

We can use $this->createPartialMock(Email::class, ['sendCommand']); for brevity. I think this is what Rector should have done.

createPartialMock() uses setMethods() that is deprecated. So using createPartialMock() changes the behavior.

createPartialMock is not deprecated. Only the implementation detail is deprecated. To this, it is not our job to check if the hidden implementation detail uses deprecated API if the API consuming this is not deprecated.

setMethods is effectively an onlyMethods, only in that onlyMethods check method existence first.

In PHPUnit 10, createPartialMock will use onlyMethods.
https://github.com/sebastianbergmann/phpunit/blob/52357158d80913d448a2ed89b54240f9d6fc07c3/src/Framework/TestCase.php#L1380-L1396

@kenjis
Copy link
Member Author

kenjis commented Jun 25, 2022

@paulbalandan We and rector cannot use createPartialMock() in this case.
Because I'm using onlyMethods(), not setMethods().
In PHPUnit9 createPartialMock() has a bug due to historical reasons, and will be fixed in PHPUnit10.

@kenjis kenjis force-pushed the fix-email-Uncaught-ErrorException branch from b2c0e22 to b872d7b Compare June 29, 2022 23:33
@kenjis
Copy link
Member Author

kenjis commented Jun 29, 2022

Rebased.

@MGatner
Copy link
Member

MGatner commented Jun 30, 2022

I appreciate all the conversation and effort to figure out these complicated tests. I also want to acknowledge that Email should be deprecated in 4.3 and is already a bit of a shabby class. If we need to hack the class or skip testing altogether I'm fine with it.

@kenjis kenjis merged commit a1e8197 into codeigniter4:develop Jul 1, 2022
@kenjis kenjis deleted the fix-email-Uncaught-ErrorException branch July 1, 2022 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Email SMTP may throw Uncaught ErrorException
4 participants