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

Implemented extra config builder extension point #102

Open
wants to merge 3 commits into
base: 1.x
Choose a base branch
from

Conversation

bdunogier
Copy link
Contributor

Third parties can register extra configuration, given a prefix and an ExtraConfigBuilder.
All commands starting with the given prefix will be sent to the matching builder, that will return a custom ExtraSiteConfig object.

Example:

$configBuilder->addExtraConfigBuilder('paywall', new PaywallExtraConfigBuilder());

Implementors are responsible for defining their own ExtraSiteConfig object, as well as their own
ExtraConfigBuilder.

Third parties can register extra configuration, given a prefix and an ExtraConfigBuilder.
All commands starting with the given prefix will be sent to the matching builder,
that will return a custom ExtraSiteConfig object.

Implementors are responsible for defining their own ExtraSiteConfig object, as well as their own
ExtraConfigBuilder.
@coveralls
Copy link

Coverage Status

Coverage decreased (-3.0%) to 95.565% when pulling 03d19d2 on bdunogier:extra_config_builder into a8501be on j0k3r:master.

Copy link
Owner

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

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

Could you add some tests with a real ExtraConfigBuilder class which returns a real SiteExtraConfig?

src/SiteConfig/ConfigBuilder.php Outdated Show resolved Hide resolved
src/SiteConfig/ExtraConfigBuilder.php Outdated Show resolved Hide resolved
src/SiteConfig/ExtraConfigBuilder.php Outdated Show resolved Hide resolved
@j0k3r
Copy link
Owner

j0k3r commented May 15, 2017

Also, don't forget to run php vendor/bin/php-cs-fixer fix too.

@bdunogier
Copy link
Contributor Author

I didn't really clean it up before sending it. The idea was to let you see the big picture.
I'll take care of it.

@bdunogier
Copy link
Contributor Author

Could you add some tests with a real ExtraConfigBuilder class which returns a real SiteExtraConfig?

I will. Review remarks fixed in the meanwhile.

} else {
$extraConfigItems[$prefix][$command][] = $val;
}
continue 2;
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this be continue since you remove one level with the first if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. A single continue would continue from foreach (array_keys($this->extraConfigBuilders) as $prefix). As we have found an extra config matching the prefix, we want to continue from the next line (foreach ($lines as $line)).

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.07%) to 95.482% when pulling 9276204 on bdunogier:extra_config_builder into a8501be on j0k3r:master.

@j0k3r j0k3r changed the base branch from master to 1.x May 22, 2019 11:52
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

3 participants