Use email address validator from Zend #8259
Conversation
|
Feedback:
|
|
@mattab Done |
| 'mx' => false, | ||
| 'deep' => false | ||
| )); | ||
| } |
There was a problem hiding this comment.
Instead of using a static variable here, DI should be used. You can get an object from DI via,
StaticContainer::get('Zend_Validate_EmailAddress'); // full class name goes here
If there's no definition in a PHP config file (ie, config/global.php or one of the plugin ones), the DI container will create a new instance itself, autowiring dependencies.
Since you're creating a instance w/ a specific constructor arg, you'll have to add a definition for the instance in config/global.php. Should be self-explanatory how to do it, let me know if it isn't.
Regarding StaticContainer use: since Piwik's internals are so deeply coupled, we have to use something like this (which stores the container as a global static variable), however we're getting close to removing it. In the future, if possible, try not to use it (by specifying dependencies in the constructor). In this case, since it's a static method, there's no choice but to use it (especially since it's @api). If this isn't clear, let me know, I'll try to clarify.
There was a problem hiding this comment.
@diosmosis StaticContainer is martked as @deprecated.
There was a problem hiding this comment.
It was introduced as @deprecated because Piwik's dependencies are too hard-coded. It's taken us ~5 months or so to try to remove it, and we are still one month away from doing it. You can use it for now, though you should try to avoid where possible.
There was a problem hiding this comment.
@diosmosis I'm worried, that if I put Zend_Validate_EmailAddress in DI, then another core/plugins developers will call email validation using Zend_Validate_EmailAddress from DI, instead of using Piwik::isValidEmailString(), and in this way Piwik will depends on Zend/Validate library. I gues this case is not about DI pattern, it's about https://en.wikipedia.org/wiki/Flyweight_pattern
But if this is a common practice for Piwik to use DI container as Flyweighter, then I can do some refactoring. Benaka, are you 100% sure about it?
There was a problem hiding this comment.
Most of what's in DI is currently not API, and when things are made API they are explicitly mentioned in docs (or should be).
As for the flyweight pattern, that normally involves more than one object. This is caching a single instance for the current request. If you don't want to use DI, you could use the transient cache, via, Piwik\Cache::getTransientCache().
Whatever you do, please make sure this value can be reset during tests, and if possible, overridden (though I think this is only do-able through DI). Static local variables have made me waste days debugging tests, and I would gladly anger the design pattern gods or whatever entity is responsible for programming zealotry if it meant I didn't have to waste my time on pointless work.
|
@diosmosis I've renamed test methods, and changed test parent class to |
|
Moved |
Mow email validation with different email formats is tested in EmailValidatorTest. UsersManagerTest::testAddUserWrongEmail() test is just about exception throwing.
|
It was a wrong old test method
|
diosmosis
added
the
not-in-changelog
Fixes #7298, use email address validator from Zend in Piwik::isValidEmail.
…ince we don't need to specify custom constructor parameters. php-di will just default to default construction.
| 'Piwik\EventDispatcher' => DI\object()->constructorParameter('observers', DI\get('observers.global')) | ||
| 'Piwik\EventDispatcher' => DI\object()->constructorParameter('observers', DI\get('observers.global')), | ||
|
|
||
| 'Zend_Validate_EmailAddress' => DI\object('Zend_Validate_EmailAddress'), |
There was a problem hiding this comment.
FYI this line is not necessary: by default with autowiring PHP-DI can create the instance without configuration needed (because here there is no constructor parameter to configure). It's no big deal but it's just to let you know
Closes #7298