Skip to content

[BUG] Adds normalize to boolean adapter#434

Closed
Jonathon-Meney-Torq wants to merge 1 commit into
pimcore:2.5from
TorqIT:fix/boolean-adapter-classification-store
Closed

[BUG] Adds normalize to boolean adapter#434
Jonathon-Meney-Torq wants to merge 1 commit into
pimcore:2.5from
TorqIT:fix/boolean-adapter-classification-store

Conversation

@Jonathon-Meney-Torq

Copy link
Copy Markdown
Contributor

Changes in this pull request

Fixes issue where checkbox classification store keys would fail on index update as value was being read as string instead of boolean.

Additional info

On a typical class definition a boolean field would be a tinyint and there of the correctly expected type. A classification store value is stored as a longtext and there fore interpreted as a string.

@sonarqubecloud

Copy link
Copy Markdown

Comment on lines +30 to +33
public function normalize(mixed $value): mixed
{
return $value !== null ? (bool) $value : null;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function normalize(mixed $value): mixed
{
return $value !== null ? (bool) $value : null;
}
public function normalize(mixed $value): mixed
{
if ($value === null) {
return null;
}
if ($this->fieldDefinition instanceof BooleanSelect) {
return $this->fieldDefinition->getDataFromResource($value);
}
return (bool) $value;
}

The boolean adapter is also registered for booleanSelect, which has a 3 states. Maybe something like this would be better? What do you think?

@MarcoLeko MarcoLeko Jul 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The is no fieldDefinition value existing for the $this context. In AdapterInterface this field is private

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

True, getFieldDefintion would be the way to go.

@MarcoLeko

Copy link
Copy Markdown

Could someone kindly take over this MR? - Im facing the the same issue currently

@mcop1

mcop1 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Could someone kindly take over this MR? - Im facing the the same issue currently

Unfortunately I dont have the permissions to push directly to the fork, but I will discuss it internally and see what we can do.

@mcop1

mcop1 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Thank you @Jonathon-Meney-Torq for the fix and analysis! Since the head branch lives on an organization-owned fork, "allow edits by maintainers" is not available and we could not push the agreed review follow-up here. We have recreated this PR upstream as #469 — your commit is included with authorship preserved, plus a follow-up commit resolving the booleanSelect tri-state point from the review thread and unit tests. Closing in favor of #469. cc @MarcoLeko

@mcop1 mcop1 closed this Jul 20, 2026
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 20, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants