diff --git a/src/Contract/ValueCandidates.php b/src/Contract/ValueCandidates.php index 1cd2d6fd..0d46072d 100644 --- a/src/Contract/ValueCandidates.php +++ b/src/Contract/ValueCandidates.php @@ -2,6 +2,11 @@ namespace ipl\Html\Contract; +/** + * @deprecated Only {@see \ipl\Html\FormElement\PasswordElement} ever used this even + * though {@see \ipl\Html\FormElement\BaseFormElement} implements it. No replacement + * will be provided. + */ interface ValueCandidates { /** diff --git a/src/Form.php b/src/Form.php index a242fc3d..6121bf68 100644 --- a/src/Form.php +++ b/src/Form.php @@ -311,6 +311,18 @@ public function isValid() return $this->isValid; } + /** + * Get whether the form has already been validated + * + * Unlike {@see isValid()}, this never triggers validation. + * + * @return bool + */ + public function hasBeenValidated() + { + return $this->isValid !== null; + } + /** * Validate all elements * diff --git a/src/FormElement/BaseFormElement.php b/src/FormElement/BaseFormElement.php index 6bb3c48d..d49a64c6 100644 --- a/src/FormElement/BaseFormElement.php +++ b/src/FormElement/BaseFormElement.php @@ -8,7 +8,6 @@ use ipl\Html\Contract\DecorableFormElement; use ipl\Html\Contract\FormElement; use ipl\Html\Contract\FormElementDecoration; -use ipl\Html\Contract\ValueCandidates; use ipl\Html\Form; use ipl\Html\FormDecoration\DecoratorChain; use ipl\Html\FormDecoration\FormElementDecorationResult; @@ -22,7 +21,7 @@ * * @phpstan-import-type decoratorsFormat from DecoratorChain */ -abstract class BaseFormElement extends BaseHtmlElement implements FormElement, ValueCandidates, DecorableFormElement +abstract class BaseFormElement extends BaseHtmlElement implements FormElement, DecorableFormElement { use Messages; use Translation; @@ -51,9 +50,6 @@ abstract class BaseFormElement extends BaseHtmlElement implements FormElement, V /** @var mixed Value of the element */ protected $value; - /** @var array Value candidates of the element */ - protected $valueCandidates = []; - /** @var ?DecoratorChain All registered decorators */ protected ?DecoratorChain $decorators = null; @@ -249,18 +245,6 @@ public function setValue($value) return $this; } - public function getValueCandidates() - { - return $this->valueCandidates; - } - - public function setValueCandidates(array $values) - { - $this->valueCandidates = $values; - - return $this; - } - public function onRegistered(Form $form) { } diff --git a/src/FormElement/FileElement.php b/src/FormElement/FileElement.php index d9ca8fd9..1843b641 100644 --- a/src/FormElement/FileElement.php +++ b/src/FormElement/FileElement.php @@ -224,6 +224,16 @@ protected function storeFiles(UploadedFileInterface ...$files): array continue; } + if (isset($this->files[$name]) && is_file($path)) { + // The file has already been stored, e.g. because setValue() was called more than + // once for the same upload ({@see FormElements::registerElement()} replays every + // populated value). Re-moving the original upload would fail as its stream has + // already been moved. + $storedFiles[] = $this->files[$name]; + + continue; + } + $file->moveTo($path); // Re-created to ensure moveTo() still works if called externally diff --git a/src/FormElement/FormElements.php b/src/FormElement/FormElements.php index f711af89..1456c356 100644 --- a/src/FormElement/FormElements.php +++ b/src/FormElement/FormElements.php @@ -8,7 +8,6 @@ use ipl\Html\Contract\FormElement; use ipl\Html\Contract\FormElementDecoration; use ipl\Html\Contract\FormElementDecorator; -use ipl\Html\Contract\ValueCandidates; use ipl\Html\Form; use ipl\Html\FormDecoration\DecoratorChain; use ipl\Html\FormDecorator\DecoratorInterface; @@ -240,10 +239,8 @@ public function registerElement(FormElement $element) $this->elements[$name] = $element; if (array_key_exists($name, $this->populatedValues)) { - $element->setValue($this->populatedValues[$name][count($this->populatedValues[$name]) - 1]); - - if ($element instanceof ValueCandidates) { - $element->setValueCandidates($this->populatedValues[$name]); + foreach ($this->populatedValues[$name] as $value) { + $element->setValue($value); } } diff --git a/src/FormElement/PasswordElement.php b/src/FormElement/PasswordElement.php index 6ef78b49..20284f24 100644 --- a/src/FormElement/PasswordElement.php +++ b/src/FormElement/PasswordElement.php @@ -12,56 +12,150 @@ class PasswordElement extends InputElement protected $type = 'password'; - /** @var bool Status of the form */ - protected $isFormValid = true; + /** @var mixed Stored value, preserved across DUMMYPASSWORD submissions */ + protected mixed $storedValue = null; - /** @var bool Status indicating if the form got submitted */ - protected $isFormSubmitted = false; + /** + * Value from the very first setValue() call, used as the baseline to detect a change + * + * `null` is a legitimate baseline (no pre-existing secret); {@see $seeded} distinguishes that + * from "not captured yet". + * + * @var mixed + */ + protected mixed $initialValue = null; - protected function registerAttributeCallbacks(Attributes $attributes) + /** @var bool Whether {@see $initialValue} has been captured */ + protected bool $seeded = false; + + /** + * Number of setValue() calls received so far, dummy placeholder included + * + * A pre-existing value seeded before the current request's own value lands on top of it takes + * at least two calls; an element that only ever got the request's own value never does, and + * thus has nothing worth masking. + * + * @var int + */ + protected int $assignments = 0; + + /** @var ?Form The form this element is registered with */ + protected ?Form $form = null; + + public function getValue() { - parent::registerAttributeCallbacks($attributes); + if ($this->hasFailedValidation()) { + return null; + } - $attributes->registerAttributeCallback( - 'value', - function () { - if ( - $this->hasValue() - && count($this->getValueCandidates()) === 1 - && $this->isFormValid - && ! $this->isFormSubmitted - ) { - return self::DUMMYPASSWORD; - } - - if (parent::getValue() === self::DUMMYPASSWORD && count($this->getValueCandidates()) > 1) { - return self::DUMMYPASSWORD; - } - - return null; - } - ); + return $this->storedValue; } - public function onRegistered(Form $form) + /** + * Get whether a changed value has failed to validate + * + * Peeks at an already known form validity only, never asks for it. Calling {@see isValid()} + * would trigger validation, which may lead to an infinite loop. + * + * @return bool + */ + protected function hasFailedValidation(): bool { - $form->on(Form::ON_VALIDATE, function ($form) { - $this->isFormValid = $form->isValid(); - }); + $value = parent::getValue(); - $form->on(Form::ON_SENT, function ($form) { - $this->isFormSubmitted = $form->hasBeenSent(); - }); + return $value !== null + && $value !== static::DUMMYPASSWORD + && ( + $this->valid === false + || ($this->form !== null && $this->form->hasBeenValidated() && ! $this->form->isValid()) + ); } - public function getValue() + public function setValue($value) { + $this->assignments++; + parent::setValue($value); $value = parent::getValue(); - $candidates = $this->getValueCandidates(); - while ($value === self::DUMMYPASSWORD) { - $value = array_pop($candidates); + if ($value !== static::DUMMYPASSWORD) { + $this->storedValue = $value; + + if (! $this->seeded) { + $this->seeded = true; + $this->initialValue = $value; + } + } + + return $this; + } + + /** + * Validate the element using all registered validators + * + * The dummy placeholder isn't a real password, just a marker for "unchanged", so it's exempt + * from validators meant for freshly typed input. + * + * @return $this + */ + public function validate(): static + { + $this->ensureAssembled(); + + if (parent::getValue() === static::DUMMYPASSWORD) { + $this->valid = true; + $this->clearMessages(); + + return $this; } - return $value; + return parent::validate(); + } + + /** + * Get the value to render into the `value` attribute + * + * In order: + * - Nothing stored, or a changed value that just failed validation: `null`. + * - No request to compare against yet (e.g. an initial GET render): masked with + * {@see DUMMYPASSWORD}. + * - Nothing was seeded before this request's own value, i.e. no secret to protect (see + * {@see $assignments}): shown while still editing, cleared once actually submitted. + * - Otherwise: masked if unchanged or actually submitted, shown while still editing. + * + * @return ?string + */ + public function getValueAttribute() + { + if ($this->storedValue === null) { + return null; + } + + $stillEditing = $this->form !== null && ! $this->form->hasBeenSubmitted(); + if (! $stillEditing && $this->hasFailedValidation()) { + return null; + } + + if ($this->form === null || ! $this->form->hasBeenSent()) { + return static::DUMMYPASSWORD; + } + + if ($this->assignments < 2) { + return $stillEditing ? $this->storedValue : null; + } + + $unchanged = $this->storedValue === $this->initialValue; + + return $unchanged || ! $stillEditing ? static::DUMMYPASSWORD : $this->storedValue; + } + + public function onRegistered(Form $form) + { + parent::onRegistered($form); + $this->form = $form; + } + + protected function registerAttributeCallbacks(Attributes $attributes) + { + parent::registerAttributeCallbacks($attributes); + $attributes->registerAttributeCallback('value', $this->getValueAttribute(...), $this->setValue(...)); } } diff --git a/tests/FormElement/FileElementTest.php b/tests/FormElement/FileElementTest.php index 8936959b..159052ff 100644 --- a/tests/FormElement/FileElementTest.php +++ b/tests/FormElement/FileElementTest.php @@ -143,6 +143,52 @@ protected function assemble() $this->assertNull($thirdForm->getValue('test')); } + public function testUploadedFileIsStoredOnlyOnceWhenSetValueIsReplayed() + { + // FormElements::registerElement() replays every populated value through setValue(). + // A file element that is populated more than once before it is registered must not + // attempt to move its already-moved upload a second time. + + $form = new class extends Form { + protected function assemble() + { + $this->addElement('file', 'test', [ + 'destination' => sys_get_temp_dir() + ]); + } + }; + + $file = new UploadedFile( + Utils::streamFor('lorem ipsum dolorem'), + 19, + 0, + 'test.txt', + 'text/plain' + ); + + $storedPath = implode(DIRECTORY_SEPARATOR, [sys_get_temp_dir(), sha1('test.txt')]); + @unlink($storedPath); + + // Two populate() calls for the same name accumulate two values, so registerElement() + // calls setValue() twice on the same upload once the element is added during assemble(). + $form->populate(['test' => $file]); + $form->populate(['test' => $file]); + + $form->ensureAssembled(); + + $this->assertSame( + 'test.txt', + $form->getElement('test')->getValue()->getClientFilename() + ); + + $this->assertSame( + 'lorem ipsum dolorem', + $form->getValue('test')->getStream()->getContents() + ); + + @unlink($storedPath); + } + public function testUploadedFileCanBeMoved() { $form = new class extends Form {