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

Allow options to be marked as deprecated #27

Closed
colinodell opened this issue Oct 10, 2020 · 2 comments
Closed

Allow options to be marked as deprecated #27

colinodell opened this issue Oct 10, 2020 · 2 comments

Comments

@colinodell
Copy link

colinodell commented Oct 10, 2020

Problem

I would like the ability to mark certain options as being 'deprecated'. These are elements that are still currently allowed but may be removed in future versions. Using a deprecated option should cause a silenced E_USER_DEPRECATED error to triggered when used.

Proposed implementation

Add a new method called deprecated() to the Base trait - something like this:

/**
 * Marks this option as deprecated
 *
 * @param ?string $message Optional deprecation message (any '%s' in the string will be replaced with the option path)
 *
 * @return $this
 */
public function deprecated(?string $message = null): self
{
    $this->deprecated = $message ?? "Option '%s' is deprecated";
    return $this;
}

Users can then flag options as deprecated like so:

 $schema = Expect::structure([
-    'foo' => Expect::string(),
+    'foo' => Expect::string()->deprecated('"%s" was deprecated in v2.1 and will be removed in v3.0; use "bar" instead'),
+    'bar' => Expect::string(),
 ]);

At some point during the complete() method call we'd raise a silenced deprecation error if any value was provided:

// TODO: Replace $valueWasProvided with the actual logic needed
if ($valueWasProvided && $this->deprecated !== null) {
    $s = implode("', '", array_map(function ($key) use ($context) {
        return implode(' › ', array_merge($context->path, [$key]));
    }, $hint ? [$extraKeys[0]] : $extraKeys));

   @trigger_error(sprintf($this->deprecated, $s), E_USER_DEPRECATED);
}

Why a silenced error?

The @trigger_error('...', E_USER_DEPRECATED) pattern is borrowed from Symfony and other projects:

Without the @-silencing operator, users would need to opt-out from deprecation notices. Silencing swaps this behavior and allows users to opt-in when they are ready to cope with them (by adding a custom error handler like the one used by the Web Debug Toolbar or by the PHPUnit bridge).

See https://symfony.com/doc/4.4/contributing/code/conventions.html#deprecating-code for more details.

Outstanding questions

  • Should an exception be thrown if a deprecated option lacks a default or is marked as required? I don't think so, but I'm not sure.
  • Should there be an alternate way to detect deprecated options? Symfony's approach works really well but not everyone uses custom error handlers that would detect these. I don't personally need an alternate method but perhaps we could record them in the Context too if that's desired?

Am I willing to implement this?

Yes - but I could use a little guidance on the best place to put the deprecation check and how to properly determine if a value is given or omitted.

@dg
Copy link
Member

dg commented Oct 14, 2020

I tried to implement it. I prefer to not use a custom error handler to detect warnings here, so I added Processor::getWarnings().

$processor = new Processor;
$schema = Expect::structure([
	'old' => Expect::int()->deprecated('The option %path% is deprecated'),
]);
$processor->process($schema, ['old' => 1]); 
var_dump($processor->getWarnings()); // ["The option 'old' is deprecated"]

Theoretically, this creates room for other possible warnings, not only for deprecation notes.

@colinodell
Copy link
Author

That implementation works for me. Thanks so much!

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

No branches or pull requests

2 participants