IMAP: Resource object constructor and stub #6534
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Girgias
requested changes
Dec 23, 2020
Member
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
Thank you for this :D
Could you add a separate test checking that one cannot extend the class?
ext/imap/tests/imap_constructor.phpt
Outdated
Comment on lines
4
to
5
| <?php | ||
| require_once __DIR__ . '/setup/skipif.inc'; |
Member
There was a problem hiding this comment.
Just a nitpick but this test doesn't need to check for the presence of a mailbox, so could you change this to
Suggested change
| <?php | |
| require_once __DIR__ . '/setup/skipif.inc'; | |
| <?php | |
| extension_loaded('imap') or die('skip imap extension not available in this build'); |
Member
Author
There was a problem hiding this comment.
Thank you, I just force-pushed an updated commit with this test simplified like you suggested.
Disallows constructing an `IMAPConnection` class directly with `new IMAPConnection` construct, by throwing an `Error` exception if attempted. `imap_open` is still the only way to create `IMAPConnection` objects.
Updates the `IMAPConnection` class stub to make sure it has the `final` flag, and adds a test to verify it.
Ayesh
force-pushed
the
imap-resources
branch
from
3889cf8 to
adb338e
Compare
Member
Author
|
Thanks a lot for the review @Girgias. I update both commits and force-pushed with updated test for constructor check, and a new test for a Thank you. |
Girgias
approved these changes
Dec 23, 2020
Member
Girgias
left a comment
Girgias
left a comment
There was a problem hiding this comment.
LGTM, will wait for CI to clear before merging. :)
Member
Member
Author
|
Thanks a lot @Girgias 🙏🏼 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is similar to #6533 (
FTPConnection), but forIMAPConnectionresource class.As of now, direct construction of
IMAPConnectionclass is allowed, and I think it was meant to be disallowed just like other new resource objects. In this PR:new IMAPConnectionconstructor with\ErrorexceptionCannot directly construct IMAPConnection, use imap_open() instead.IMAPConnectionwithfinalflag.Thank you.