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

Contexts #16

Merged
merged 4 commits into from
Nov 15, 2021
Merged

Contexts #16

merged 4 commits into from
Nov 15, 2021

Conversation

MGatner
Copy link
Member

@MGatner MGatner commented Nov 14, 2021

This PR expands the core function to add a new feature: contexts. A "contextual setting" is a way of defining a setting for a specific subset of circumstances, left to the developer to define (see #15 (comment)). Contextual settings fall back on their "global" version (i.e. the current behavior), providing one more layer of customizability. An example of a contextual setting might be environment-specific URLs:

service('Settings')->set('App.homepage', '/welcome', 'environment:production');
service('Settings')->set('App.homepage', '/debug', 'environment:development');

In order to facilitate detection of "hits" versus "misses" this PR also adds the has() method to handlers to determine whether a value is present or not. This expands on null value support, since we can now differentiate between "nonexistent" and "present but null".

Note: Because this feature modifies an abstract method definition it will require a major release.

@MGatner
Copy link
Member Author

MGatner commented Nov 15, 2021

The BC check failure is expected, as noted.

@MGatner MGatner requested a review from lonnieezell November 15, 2021 00:22
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@@ -142,7 +163,7 @@ private function hydrate()
$rawValues = db_connect()->table($this->table)->get();
Copy link
Member

Choose a reason for hiding this comment

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

I think our hydration system and caching system will need to change. If we have thousands of users in the system we don't want to load all of their individual settings into the system at once. I would recommend that hydrate could take the $context. If present it caches it locally under the context name, otherwise keeps another cache for non-context settings.

private $settings = [
   'general' => [],
   'contexts' => []
];

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea!

@sfadschm
Copy link

sfadschm commented Nov 15, 2021

If I define a contraint that does not match, but have a default value defined in a config file, will settings fallback to the config or null?

On the same note:
If I define a setting twice, once with and once without constraint, then I guess it will fallback to the contraint-less value if not matching, right?

@MGatner
Copy link
Member Author

MGatner commented Nov 15, 2021

That's correct. It is a cascading check, so if you supply a context then the return is "setting with context > setting without context > Config value > null".

I tried to make this clear in the README:

If that context is not found the library falls back to the global value, i.e. service('setting')->get('App.theme')

@sfadschm Did you miss that, or did you read that and still found it confusing?

@sfadschm
Copy link

Did you miss that, or did you read that and still found it confusing?

Miss...
However, I like the phrasing in your answer. Maybe explicitly stating the cascade chain could be helpful in the docs (although it should in principle be understandable the way it is now).

@MGatner MGatner requested a review from lonnieezell November 15, 2021 15:12
@MGatner MGatner merged commit 6a26ae8 into develop Nov 15, 2021
@MGatner MGatner deleted the context branch November 15, 2021 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants