Allow options to be marked as deprecated

colinodell commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

That implementation works for me. Thanks so much!

nette / schema