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

FormElement: Add Multiselect element #65

Closed
wants to merge 10 commits into from
57 changes: 57 additions & 0 deletions src/FormElement/MultiselectElement.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
<?php

namespace ipl\Html\FormElement;

class MultiselectElement extends SelectElement
{
/** @var array Selected values */
protected $value = [];

public function getValueAttribute()
{
// select elements don't have a value attribute
return null;
}

public function getNameAttribute()
{
return $this->getName() . '[]';
}

public function setValue($value)
{
$this->value = empty($value) ? [] : (array) $value;
$this->valid = null;

return $this;
}

public function validate()
{
foreach ($this->getValue() as $value) {
$option = $this->getOption($value);
if (! $option || $option->getAttributes()->has('disabled')) {
$this->valid = false;
$this->addMessage(sprintf($this->translate("'%s' is not allowed here"), $value));

return $this;
}
}

BaseFormElement::validate();
Copy link
Member

Choose a reason for hiding this comment

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

Why not parent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the parent class checks its own value first, since I already have a loop above for this check, I skip calling the parent method.

Copy link
Member

Choose a reason for hiding this comment

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

Exactly. In addition we have duplicated code. This shows that the code flow is flawed, and it raises the question of why we even have a separate element for this. Instead, specifying the multiple attribute for the select element should just work.


return $this;
}

protected function isSelectedOption($optionValue)
{
return in_array($optionValue, $this->getValue(), ! is_int($optionValue));
}

protected function assemble()
{
$this->setAttribute('multiple', true);

parent::assemble();
}
}
42 changes: 28 additions & 14 deletions src/FormElement/SelectElement.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@
namespace ipl\Html\FormElement;

use ipl\Html\Html;
use ipl\I18n\Translation;

class SelectElement extends BaseFormElement
{
use Translation;

protected $tag = 'select';

/** @var SelectOption[] */
Expand Down Expand Up @@ -45,13 +48,13 @@ public function validate()
)
) {
$this->valid = false;
$this->addMessage("'$value' is not allowed here");
} elseif ($this->isRequired() && $value === null) {
$this->valid = false;
} else {
parent::validate();
$this->addMessage(sprintf($this->translate("'%s' is not allowed here"), $value));
Copy link
Member

Choose a reason for hiding this comment

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

Possible candidate for InArrayValidator or DeferredInArrayValidator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have already created a new PR for this.

Copy link
Member

Choose a reason for hiding this comment

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

But I don't see its usage here.


return $this;
}

parent::validate();

return $this;
}

Expand All @@ -67,9 +70,10 @@ public function disableOption($value)
if ($option = $this->getOption($value)) {
$option->getAttributes()->add('disabled', true);
}
if ($this->getValue() == $value) {

if ($this->isSelectedOption($value)) {
$this->valid = false;
$this->addMessage("'$value' is not allowed here");
$this->addMessage(sprintf($this->translate("'%s' is not allowed here"), $value));
}

return $this;
Expand Down Expand Up @@ -124,20 +128,30 @@ protected function makeOption($value, $label)
} else {
$option = new SelectOption($value, $label);
$option->getAttributes()->registerAttributeCallback('selected', function () use ($option) {
$optionValue = $option->getValue();

return is_int($optionValue)
// The loose comparison is required because PHP casts
// numeric strings to integers if used as array keys
? $this->getValue() == $optionValue
: $this->getValue() === $optionValue;
return $this->isSelectedOption($option->getValue());
});
$this->options[$value] = $option;

return $this->options[$value];
}
}

/**
* Whether the given option is a selected option
*
* @param $optionValue
*
* @return bool
*/
protected function isSelectedOption($optionValue)
{
return is_int($optionValue)
// The loose comparison is required because PHP casts
// numeric strings to integers if used as array keys
? $this->getValue() == $optionValue
: $this->getValue() === $optionValue;
}

protected function assemble()
{
$this->addHtml(...array_values($this->optionContent));
Expand Down
Loading