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

Reducing the complexity of view helpers #259

Merged
merged 4 commits into from
Jan 9, 2024

Conversation

mimmi20
Copy link
Contributor

@mimmi20 mimmi20 commented Jan 9, 2024

Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC no
QA yes

Description

This PR wants to reduce the complexity of the FormCollection and the FormRow View Helpers. Also an psalm issue is fixed.

protected function translateLabel(string|int $label): string|int
protected function translateLabel(string $label): string
Copy link
Member

Choose a reason for hiding this comment

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

This will fix psalm issues in #260

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason int was allowed in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

I think removing 'int' causes a BC Break. For example, if we want to generate pagination and we use only numbers:

$value = 10;

/** @var \Laminas\Form\Element\Select $field */
$field = $this->get('items_per_page');

$values = [];
for ($i = 1; $i < 4; $i++) {
    $val = $i * $value;

    $values[$val] = $val;
}

$field->setValueOptions($values);

Error message:

Laminas\Form\View\Helper\AbstractHelper::translateLabel(): Argument # 1 ($label) must be of type string, int given, called in .../laminas/laminas-form/src/View/Helper/FormSelect.php on line 201

$label = $this->translateLabel($label);

The solution is the need to always remember to cast all values to string:

$values[$val] = (string) $val;

Shouldn't this change be accompanied by a change: #256?

Copy link
Member

Choose a reason for hiding this comment

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

I'm with you here. A (string) cast should be used for all calls, or translateLabel should be altered to accept scalar and perform the cast there.

Conversely, all labels should be string values regardless, so I do think it's up to the user to convert labels to strings first, but that doesn't stop this from being a BC break based on existing behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

See #261

src/View/Helper/Form.php Outdated Show resolved Hide resolved
@froschdesign froschdesign changed the title reduce complexity Reduce complexity of the FormCollection and the FormRow View Helpers Jan 9, 2024
@froschdesign froschdesign changed the title Reduce complexity of the FormCollection and the FormRow View Helpers Reducing the complexity of view helpers Jan 9, 2024
Signed-off-by: Thomas Müller <mimmi20@live.de>
Signed-off-by: Thomas Müller <mimmi20@live.de>
Signed-off-by: Thomas Müller <mimmi20@live.de>
@Xerkus
Copy link
Member

Xerkus commented Jan 9, 2024

@mimmi20 github having incident currently. You might want to wait until problem is resolved.

Copy link
Contributor

@Slamdunk Slamdunk left a comment

Choose a reason for hiding this comment

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

Reviewed with split-diff and hidden-whitespaces: LGTM

Signed-off-by: Thomas Müller <mimmi20@live.de>
Comment on lines +164 to +166
if ($label === '' || $type === 'hidden') {
return $elementString . $elementErrors;
}
Copy link
Member

Choose a reason for hiding this comment

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

Just to note.

This and following parts lost conditional for $this->renderErrors.
Behavior remains correct because $elementErrors is an empty string with render errors checked at line 155 that is not part of the diff.

@Xerkus Xerkus merged commit 1c9ac51 into laminas:3.19.x Jan 9, 2024
11 checks passed
@Xerkus
Copy link
Member

Xerkus commented Jan 9, 2024

@mimmi20 thank you

@Xerkus Xerkus added this to the 3.19.0 milestone Jan 9, 2024
@mimmi20 mimmi20 deleted the feature-reduce-complexity branch January 9, 2024 19:51
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

Successfully merging this pull request may close these issues.

None yet

5 participants