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

Fix invalid abstract service factory configuration #162

Merged
merged 2 commits into from
May 14, 2018
Merged

Fix invalid abstract service factory configuration #162

merged 2 commits into from
May 14, 2018

Conversation

codeaid
Copy link
Contributor

@codeaid codeaid commented Feb 6, 2018

This fixes #129.

Registration of InputFilterAbstractServiceFactory is currently invalid in ConfigProvider. Instead of the abstract_factories key being nested under dependencies it should be nested under input_filters.

This pull request corrects the behaviour and allows using the service with no manual changes required when using the library in a Zend Expressive project.

  • Are you fixing a bug?
    • Detail how the bug is invoked currently.
    • Detail the original, incorrect behavior.
    • Detail the new, expected behavior.
    • Base your feature on the master branch, and submit against that branch.
    • Add a regression test that demonstrates the bug, and proves the fix.
      Is this really needed? I've included tests that validate the current (correct) behaviour.
    • Add a CHANGELOG.md entry for the fix.

Copy link
Member

@michalbundyra michalbundyra left a comment

Choose a reason for hiding this comment

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

@codeaid It looks like desired behaviour, please see:
https://docs.zendframework.com/zend-inputfilter/specs/#setup

@codeaid
Copy link
Contributor Author

codeaid commented Feb 6, 2018

@webimpress Sorry, not sure what you meant. Is something wrong with the config?

@michalbundyra
Copy link
Member

@codeaid Documentation says that you have to add it manually in your configuration when you using zend-mvc v2. With v3 or expressive the configuration is slightly different and your change is not needed, imho.

@codeaid
Copy link
Contributor Author

codeaid commented Feb 6, 2018

Documenation says "when using the configuration manager [..] the functionality is enabled by default" - I registered ConfigProvider in my Expressive application (installed using the instructions on the project homepage) and I wasn't able to use the abstract factory. How does it get enabled by default then?

Edit:
Also, what's the purpose of having this section added to dependencies if it's not even invoked?

CHANGELOG.md Outdated
- Nothing.
- [#129](https://github.com/zendframework/zend-inputfilter/pull/129) fixes
incorrect abstract service factory registration in `ConfigProvider`as per
the [latest documentation](https://zendframework.github.io/zend-inputfilter/specs/#setup).
Copy link
Member

Choose a reason for hiding this comment

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

Please add the correct link for documentation: https://docs.zendframework.com/zend-inputfilter/specs/#setup

@froschdesign
Copy link
Member

Fix is okay! 👍

(Btw. the same problem in zend-form.)

Copy link
Member

@michalbundyra michalbundyra left a comment

Choose a reason for hiding this comment

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

@codeaid @froschdesign Oh, yeah... sorry you have right. The fix is right 👍 We creates another 'service manager' for input filters, and the abstract factory should be registered there.

@codeaid
Copy link
Contributor Author

codeaid commented Feb 7, 2018

Ah, I thought I was going crazy for a moment! Glad I can be of help after all.

I force-pushed the updated link in the changelog to avoid duplicate commits. Everything should be alright now.
On a side note - would you want me to fix this issue in zend-form too while I'm at it, @froschdesign?

@froschdesign
Copy link
Member

@codeaid

would you want me to fix this issue in zend-form too while I'm at it

That would be awesome! 👍

@weierophinney weierophinney merged commit a4acfb1 into zendframework:master May 14, 2018
weierophinney added a commit that referenced this pull request May 14, 2018
Additionally, `s/129/162/`, as the latter is the actual pull request
introducing the change (while the former is the issue that prompted it).
weierophinney added a commit that referenced this pull request May 14, 2018
weierophinney added a commit that referenced this pull request May 14, 2018
@weierophinney
Copy link
Member

Thanks, @codeaid!

@codeaid
Copy link
Contributor Author

codeaid commented May 18, 2018

Thanks, @weierophinney! Sorry, I haven't had time to fix this issue in zend-form. Been busy with things other than PHP.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConfigProvider - registration of InputFilterAbstractServiceFactory
4 participants