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

Add support for appending config options #72

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

duncan3dc
Copy link
Contributor

When configuring to exclude a symbol, we have to copy all the default symbols, and then keep that up to date as new symbols are added in future.

To avoid this, I propose allowing configuration to be appended to, as well as overwritten:

{
  "append": {
    "symbol-whitelist" : ["strict"],
    "php-core-extensions" : ["json"]
  }
}

@duncan3dc
Copy link
Contributor Author

Another approach I thought of, was to use a special symbol to indicate that the list should be extended, eg:

{
  "symbol-whitelist" : ["*", "strict"],
  "php-core-extensions" : ["*", "json"]
}

@bcremer
Copy link

bcremer commented Jun 18, 2018

Is it really necessary to opt-in into append mode after all, or can we just append by default?
The built-in config.dist.json would then be like an internal implementation detail for the symbols and extensions provided by the php core.

@Ocramius
Copy link
Collaborator

I don't understand the issue: can you elaborate on what this is and why it is needed?

@duncan3dc
Copy link
Contributor Author

This library currently allows you to configure the list of symbols to ignore.

However you must pass the default list the library ships with, plus your symbols.
So when the default list changes (new keywords in PHP for example), everyone that is using this feature has to change their list to include the new symbol too.

This PR allows you to append to the default list, instead of just a complete override.
It improves the configuration feature by avoiding redundant updates on each new PHP release

@maglnet
Copy link
Owner

maglnet commented Jun 26, 2018

I would prefer to remove the builtin symbols from this whitelist, so extending would not be needed anymore.

Instead of whitelisting the builtin symbols, we could possibly use the isBuiltin() method of ReflectionType and so on, or switch to BetterReflection, if this provides the same functionality.

Have a look at this for an isBuiltin() example
https://3v4l.org/TtYcC

@Ocramius
Copy link
Collaborator

BetterReflection only has minimal support for built-ins via stubbing, but it could indeed work.

As for the suggestion above, a simpler way would be to allow an invokable class to contain the built-ins, such as:

namespace Some\Defaults;

final class BuiltIns
{
    public function __invoke() : array
    {
        return [...];
    }
}

Then in the config, refer to Some\Defaults\BuiltIns, or use it from the consumer code and reference the class in the consumer code (array_merge() is a simple way to reuse this)

@maglnet
Copy link
Owner

maglnet commented Jun 26, 2018

I hope I'm getting it right, you think about adding something like adding (new Some\Defaults\BuiltIns())() to this array here?
https:/maglnet/ComposerRequireChecker/blob/master/src/ComposerRequireChecker/Cli/CheckCommand.php#L88

@Ocramius
Copy link
Collaborator

More like having the class name in the configuration 👌

@lookyman
Copy link

lookyman commented Mar 15, 2021

Hello, just interested in the status of this. I could really use it.

Also, what do you think about using composer extra for this? Would be quite convenient and would avoid having to use config file altogether.

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.

5 participants