-
Notifications
You must be signed in to change notification settings - Fork 111
Add option novalidatecert to connect() #247
base: develop
Are you sure you want to change the base?
Conversation
src/Protocol/Imap.php
Outdated
* Do not validate the SSL certificate if set to true | ||
* @var null|bool | ||
*/ | ||
public $novalidatecert; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as we do not have typed properties, we can not use public here. You can use the constructor to set a value for this property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I'd add either:
- a 4th constructor argument,
$validateCert = true
- a 4th constructor argument,
array $options
, and document a "validate_cert" or "validateCert" option.
The latter makes it easier to expand options, but leads to more validation once we add typehints. The former is self-documenting, but means if we add more options down the line, the constructor signature gets really large.
I'm fine with either approach, but agree with @froschdesign here that a public property is not a great idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi guys, thank you for the feedback!
I have added the variable to the constructor, but this does not really fix the issue. The thing is that the way you create a POP3 or IMAP connection is by creating an instance of Zend\Mail\Storage\Pop3
or Zend\Mail\Storage\Imap3
.
The constructor then calls the Zend\Mail\Protocol constructor without arguments, followed by calling connect()
.
But because I cannot add a new parameter to connect()
(see previous PR), and I have to leave the constructor call without arguments as it is, I have now added a setter method for novalidatecert
.
Please see my latest commit.
Any progress or feedback on this PR? |
As you can see there is no any new comments or approvals, so not yet :) |
This repository has been closed and moved to laminas/laminas-mail; a new issue has been opened at laminas/laminas-mail#5. |
This repository has been moved to laminas/laminas-mail. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:
|
See discussion on PR #246