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

Removes deprecation messages which lead to irritation because there are no alternative paths for implementation #77

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

froschdesign
Copy link
Member

The current deprecations of methods and properties cannot be resolved or bypassed by other options or workarounds. The isValid method of the Input class uses and needs these deprecated methods and properties:

public function isValid($context = null)
{
if (is_array($this->errorMessage)) {
$this->errorMessage = null;
}
$value = $this->getValue();
$hasValue = $this->hasValue();
$empty = $value === null || $value === '' || $value === [];
$required = $this->isRequired();
$allowEmpty = $this->allowEmpty();
$continueIfEmpty = $this->continueIfEmpty();
if (! $hasValue && $this->hasFallback()) {
$this->setValue($this->getFallbackValue());
return true;
}
if (! $hasValue && ! $required) {
return true;
}
if (! $hasValue) { // required, but no value
if ($this->errorMessage === null) {
$this->errorMessage = $this->prepareRequiredValidationFailureMessage();
}
return false;
}
if ($empty && ! $required && ! $continueIfEmpty) {
return true;
}
if ($empty && $allowEmpty && ! $continueIfEmpty) {
return true;
}

/**
* @deprecated 2.4.8 Add Laminas\Validator\NotEmpty validator to the ValidatorChain.
*
* @var bool
*/
protected $allowEmpty = false;
/**
* @deprecated 2.4.8 Add Laminas\Validator\NotEmpty validator to the ValidatorChain.
*
* @var bool
*/
protected $continueIfEmpty = false;

/**
* @deprecated 2.4.8 Add Laminas\Validator\NotEmpty validator to the ValidatorChain.
*
* @var bool
*/
protected $notEmptyValidator = false;

An alternative solution or way was never provided and in its current state, it would not work. Therefore the deprecation was too early and all discussions and developments in this direction have never been completed or are very old.

See:

As long as we do not have an alternative solution, the current deprecation messages are confusing.

…re no alternative paths for implementation

Signed-off-by: Frank Brückner <[email protected]>
@froschdesign froschdesign added the Bug Something isn't working label Oct 21, 2022
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Fine by me. Need another opinion from who introduced this deprecation

@Ocramius Ocramius added this to the 2.22.1 milestone Oct 21, 2022
Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

I agree with the decision to un-deprecate this stuff. That said, I can see why it was deprecated as the "is it empty/required" dance is convoluted to say the least and the magic of auto injecting the NotEmpty validator is questionable.

IMO it's worth raising an issue that it all needs sorting out for the next major.

Does removing the deprecations alter the baseline?

Copy link
Member

@internalsystemerror internalsystemerror left a comment

Choose a reason for hiding this comment

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

No objections here. When/if a solution is found, is there a chance these deprecations would be readded as they are?

@Ocramius
Copy link
Member

Ok, let's just re-generate the baseline, and then this can be merged.

Can't do myself: don't have docker on this PC today :D

@Ocramius Ocramius self-assigned this Nov 4, 2022
@Ocramius
Copy link
Member

Ocramius commented Nov 4, 2022

Merging as-is here: can't re-generate baseline ATM, but it's a minor issue.

@Ocramius Ocramius merged commit ec8b923 into laminas:2.22.x Nov 4, 2022
@Ocramius
Copy link
Member

Ocramius commented Nov 4, 2022

Thanks @froschdesign!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants