Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added EncapsulatedOptions::merge() and accompanying test #5

Closed
wants to merge 3 commits into from

Conversation

Bilge
Copy link
Member

@Bilge Bilge commented Oct 16, 2016

This PR implements option merging for EncapsulatedOptions such that two compatible instances may be merged into each other. This is a common use case since a provider may supply options and a resource supplies its own superset of the same options class which need to be combined before they're forwarded to the connector.

There are some interesting questions raised by this.

  1. Should the merged options become a new instance or modify one of the existing instances?
  2. Should merging consider only the options or copy in the default values as well?
    1. If it should consider defaults, should it do so for one or both instances?
  3. What should be the default merge strategy? Is a simple union sufficient or should it be recursive?
  4. How should merging be implemented in a derived class? Simple method override or strategy pattern?

Currently this PR implements the following answers to these questions, subject to change.

  1. Existing instance is mutated.
  2. Only options are considered, default values are ignored.
  3. Simple array union of options.
  4. Simple method override.

@@ -61,6 +91,10 @@
*/
final protected function set($option, $value)
{
if (is_object($value) || is_resource($value)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: This value type checking needs to be extended to recursively check array values and defaults.

@Bilge Bilge removed this from In development in TODO Mar 16, 2017
@Bilge Bilge force-pushed the master branch 5 times, most recently from 01c8862 to d8cd138 Compare March 20, 2017 00:14
@0xPaul 0xPaul added this to the 4.0.0 milestone Jun 20, 2017
@Bilge
Copy link
Member Author

Bilge commented Mar 31, 2018

This is no longer needed since #48, which removes the need to pass options around, to have multiple option instances, and therefore the need to merge instances together. No longer do we have to make guesses about how different sets of options should be merged because we only ever deal with one set of options and specific option implementations can decide how their properties should be managed, which is an altogether more robust approach.

@Bilge Bilge closed this Mar 31, 2018
@Bilge Bilge deleted the merge-options branch April 9, 2018 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants