Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

improvments for Zend\InputFilter #7392

Closed
wants to merge 3 commits into from

Conversation

kokspflanze
Copy link
Contributor

HI,

added missing methods @ Zend\InputFilter\InputInterface
added some doc-blocks in Zend\InputFilter*Interface
little code improvements, for example signature say return $this and the method return nothing

Thanks=)

added some doc-blocks in Zend\InputFilter\*Interface
little code improvements, for example signature say return $this and the method return nothing
@kokspflanze
Copy link
Contributor Author

i will fix the phpunit error tomorrow, sry=[

@@ -52,9 +49,8 @@ public function canCreateServiceWithName(ServiceLocatorInterface $inputFilters,
* @param string $rName
* @return \Zend\InputFilter\InputFilterInterface
*/
public function createServiceWithName(ServiceLocatorInterface $inputFilters, $cName, $rName)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually correct, because it is an instance of InputFilterPluginManager and not the ServiceManager itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so the method signature has to be changed to AbstractPluginManager ?
For me was the problem that the ServiceLocatorInterface doesnt have the method getServiceLocator.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it can stay just the way it was. Regarding the method getServiceLocator there already is an other issue for this and I think it depends on the outcome of that discussion whether it needs a change to AbstractPluginManager. See #7388.

…ady a discussion about that @ zendframework#7388 and the signature can not be changed to AbstractPluginManager, because the AbstractFactoryInterface does not allowed that.
@kokspflanze
Copy link
Contributor Author

thanks @Martin-P for the hint.

I revert the InputFilterAbstractServiceFactory class.

* @return self
*/
public function setContinueIfEmpty($continueIfEmpty);

Copy link
Member

Choose a reason for hiding this comment

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

This is defined in EmptyContextInterface, and is there because we cannot add to existing interfaces (causes a BC break). This is why Input implements EmptyContextInterface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use this method @ Input and in Factory , i think we have to add something to support the old InputInterface too, maybe with method_exists, or what you think about that?

@weierophinney
Copy link
Member

@kokspflanze — As noted, with the current changes, I cannot merge this as it introduces BC breaks.

If you remove the new methods from the InputInterface entirely, I can merge it for 2.4.1. If you want to move the setFallbackValue() method into a new interface, I can merge for 2.5.0.

@kokspflanze
Copy link
Contributor Author

@weierophinney thanks for the feedback, i will change it asap. =)

weierophinney added a commit that referenced this pull request May 6, 2015
@weierophinney weierophinney added this to the 2.4.1 milestone May 6, 2015
@jerv13
Copy link

jerv13 commented May 11, 2015

After updating to 2.4.1, all the Zend\Filter\Boolean filters are causing validation errors. I have not investigated in detail, but is likely related to this change.

@kokspflanze kokspflanze deleted the develop branch May 11, 2015 18:26
@kokspflanze
Copy link
Contributor Author

@jerv13 can you give an example?

weierophinney added a commit to zendframework/zend-inputfilter that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants