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

Enable event listeners via config. Tests. Docs. #62

Merged
merged 8 commits into from
Dec 28, 2019
Merged

Enable event listeners via config. Tests. Docs. #62

merged 8 commits into from
Dec 28, 2019

Conversation

marcguyer
Copy link
Contributor

@marcguyer marcguyer commented Jul 30, 2019 •
edited
Loading

Provide a narrative description of what you are trying to accomplish:

  • Are you creating a new feature?
    • Why is the new feature needed? What purpose does it serve?
    • How will users use the new feature?
    • Base your feature on the develop branch, and submit against that branch.
    • Add only one feature per pull request; split multiple features over multiple pull requests
    • Add tests for the new feature.
    • Add documentation for the new feature.
    • Add a CHANGELOG.md entry for the new feature.

Adds a config option (listeners). Using the new config array, a user may choose to attach 1 or more listeners to listen for events emitted by the authorization server.

geerteltink
geerteltink reviewed Aug 5, 2019
View reviewed changes
froschdesign
froschdesign reviewed Aug 5, 2019
View reviewed changes
@marcguyer
Copy link
Contributor Author

Guys, sorry for the feature expansion... It's still the same feature, just more robust. Here's the breakdown of what I did:

  • Agreed that we should return an empty array when no config
  • Agreed that docs should be improved
  • After additional study of league/event I decided to also enable config of implementations of League\Event\ListenerProviderInterface since that's another way of adding listeners that I initially missed.

Something of note is the convention for config keys. I'm not sure of the latest ZF/ZE convention for config keys. I've been using hyphens in my stuff but there's some crossover in the various recent ZE packages. Has the convention been documented somewhere? My guess is that underscores are preferred so I went ahead and changed to use them instead of hyphens.

@marcguyer marcguyer force-pushed the feature/eventListenerConfig branch from 7a41fe8 to 6657881 Compare September 28, 2019 14:10
@marcguyer marcguyer changed the base branch from master to develop September 28, 2019 14:19
@marcguyer
Copy link
Contributor Author

Hi guys -- When I started this PR, the develop branch did not exist. Now that it does and the travis config has been fixed, I went ahead and set develop as the base of my feature branch and also changed the base of the PR to develop. This looks good to go from my perspective -- looking for some feedback. Thanks.

@michalbundyra michalbundyra added this to the 1.3.0 milestone Sep 28, 2019
marcguyer and others added 8 commits December 28, 2019 13:59
@marcguyer @weierophinney
Enable event listeners via config. Tests. Docs.
242406e
@marcguyer @weierophinney
Add support for listener providers config
49a0cf7 
* Remove commented out example listeners config
* Rewrite docs with subheadings and better example configs
* Empty config returns empty array instead of null
* Add some basic error handling for missing container services
* Tests
@marcguyer @weierophinney
Fix cs errors
9361dd2
@marcguyer @weierophinney
Remove commented-out listener config example
43311f0
@marcguyer @weierophinney
Use underscores for config keys
68c8c2c
@weierophinney
docs: note that configurable event listeners are from 1.3.0 forward
db8cc1b
@weierophinney
qa: CS fixes for AuthorizationServerFactoryTest
08e5e5f 
- trailing commas in array entries
- consistent whitespace
@weierophinney
docs: adds CHANGELOG entry for zendframework#62
93ddf21
@weierophinney weierophinney force-pushed the feature/eventListenerConfig branch from 6657881 to 93ddf21 Compare December 28, 2019 20:08
@weierophinney
Copy link
Member

I've rebased against current develop, added a note to the docs about the feature being from version 1.3.0+, and added some minor CS changes; will merge and release shortly!

Thanks, @marcguyer !

@weierophinney weierophinney merged commit 93ddf21 into zendframework:develop Dec 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@geerteltink geerteltink geerteltink approved these changes

@froschdesign froschdesign Awaiting requested review from froschdesign

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.
5 participants
@marcguyer @weierophinney @froschdesign @geerteltink @michalbundyra