Skip to content

fix: Update type comparisons to strict equality for CustomAttributeTy… #974

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.

Sign up for GitHub

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

Draft
JohnVillalovos wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

fix: Update type comparisons to strict equality for CustomAttributeTy… #974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Domain/Access/AttributeFilter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ public static function Create($entityTableAndColumn, $attributes)

$idEquals = new SqlFilterEquals($attributeId, $attribute->Id());
$f->AppendSql('LEFT JOIN `' . TableNames::CUSTOM_ATTRIBUTE_VALUES . '` `a' . $id . '` ON `a0`.`entity_id` = `a' . $id . '`.`entity_id` ');
if ($attribute->Type() == CustomAttributeTypes::MULTI_LINE_TEXTBOX || $attribute->Type() == CustomAttributeTypes::SINGLE_LINE_TEXTBOX) {
if ((int)$attribute->Type() === CustomAttributeTypes::MULTI_LINE_TEXTBOX || (int)$attribute->Type() === CustomAttributeTypes::SINGLE_LINE_TEXTBOX) {
$attributeFragment->_And($idEquals->_And(new SqlFilterLike($attributeValue, $attribute->Value())));
} elseif ($attribute->Type() == CustomAttributeTypes::CHECKBOX && $attribute->Value() == '0') {
} elseif ((int)$attribute->Type() === CustomAttributeTypes::CHECKBOX && $attribute->Value() == '0') {
$attributeFragment->_And(new SqlFilterFreeForm('NOT EXISTS (SELECT 1 FROM `' . TableNames::CUSTOM_ATTRIBUTE_VALUES . '` `b` WHERE `b`.`entity_id` = `a0`.`entity_id` AND `b`.`custom_attribute_id` = ' . $id . ')'));
} else {
$attributeFragment->_And($idEquals->_And(new SqlFilterEquals($attributeValue, $attribute->Value())));
Expand Down
6 changes: 3 additions & 3 deletions Domain/CustomAttribute.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ public function __construct(
$this->category = $category;
$this->SetRegex($regex);
$this->required = $required;
if ($category != CustomAttributeCategory::RESERVATION) {
if ((int)$category !== CustomAttributeCategory::RESERVATION) {
$this->entityIds = is_array($entityIds) ? $entityIds : ($entityIds);
}
$this->adminOnly = $adminOnly;
Expand Down Expand Up @@ -452,7 +452,7 @@ public function Update($label, $regex, $required, $possibleValues, $sortOrder, $
$this->SetRegex($regex);
$this->required = $required;

if ($this->category != CustomAttributeCategory::RESERVATION) {
if ((int)$this->category !== CustomAttributeCategory::RESERVATION) {
$entityIds = is_array($entityIds) ? $entityIds : [$entityIds];
$removed = array_diff($this->entityIds, $entityIds);
$added = array_diff($entityIds, $this->entityIds);
Expand Down Expand Up @@ -503,7 +503,7 @@ public function WithEntityDescriptions($entityDescriptions)
*/
public function WithSecondaryEntities($category, $entityIds, $entityDescriptions = null)
{
if ($this->category != CustomAttributeCategory::RESERVATION) {
if ((int)$this->category != CustomAttributeCategory::RESERVATION) {
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

WithSecondaryEntities uses (int)$this->category != ... which is still a non-strict comparison. For consistency with the rest of this PR (and to avoid loose-comparison surprises), change this to strict !== (keeping the cast if needed).

Suggested change
if ((int)$this->category != CustomAttributeCategory::RESERVATION) {
if ((int)$this->category !== CustomAttributeCategory::RESERVATION) {

Copilot uses AI. Check for mistakes.
return;
}

Expand Down
10 changes: 5 additions & 5 deletions Pages/Admin/ManageAttributesPage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@ public function GetLabel();

/**
* @abstract
* return int|CustomAttributeTypes
* return CustomAttributeTypes
*/
public function GetType();

/**
* return int|CustomAttributeCategory
* return CustomAttributeCategory
*/
public function GetCategory();
Comment on lines 14 to 23
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

These interface docblocks use plain return ... text instead of @return, so they likely won’t be interpreted by PHPStan/IDEs as type information. If the intent is to document types, switch to proper @return tags and use int (or int|CustomAttributeTypes / int|CustomAttributeCategory) since these are classes of constants rather than real value object/enum types.

Copilot uses AI. Check for mistakes.

Expand All @@ -43,7 +43,7 @@ public function GetEntityIds();
public function GetPossibleValues();

/**
* return int|CustomAttributeCategory
* return CustomAttributeCategory
*/
public function GetRequestedCategory();

Expand All @@ -63,7 +63,7 @@ public function GetIsAdminOnly();
public function BindAttributes($attributes);

/**
* @param $categoryId int|CustomAttributeCategory
* @param $categoryId CustomAttributeCategory
*/
public function SetCategory($categoryId);

Expand All @@ -78,7 +78,7 @@ public function GetAttributeId();
public function GetSecondaryEntityIds();

/**
* @return CustomAttributeCategory|int|null
* @return CustomAttributeCategory|null
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

GetSecondaryCategory() is a form value ($this->GetForm(...)) and may be a string/int. Declaring the return type as CustomAttributeCategory|null is misleading because CustomAttributeCategory is not an enum/object type here. Prefer int|null (or int|CustomAttributeCategory|null if matching existing conventions).

Suggested change
* @return CustomAttributeCategory|null
* @return int|null

Copilot uses AI. Check for mistakes.
*/
public function GetSecondaryCategory();

Expand Down
2 changes: 1 addition & 1 deletion Pages/Admin/ManageResourcesPage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1323,7 +1323,7 @@ public function AsFilter($customAttributes)

$idEquals = new SqlFilterEquals($attributeId, $id);
$f->AppendSql('LEFT JOIN `' . TableNames::CUSTOM_ATTRIBUTE_VALUES . '` `a' . $id . '` ON `a0`.`entity_id` = `a' . $id . '`.`entity_id` ');
if ($attribute->Type() == CustomAttributeTypes::MULTI_LINE_TEXTBOX || $attribute->Type() == CustomAttributeTypes::SINGLE_LINE_TEXTBOX) {
if ((int)$attribute->Type() === CustomAttributeTypes::MULTI_LINE_TEXTBOX || (int)$attribute->Type() === CustomAttributeTypes::SINGLE_LINE_TEXTBOX) {
$attributeFragment->_And($idEquals->_And(new SqlFilterLike($attributeValue, $value)));
} else {
$attributeFragment->_And($idEquals->_And(new SqlFilterEquals($attributeValue, $value)));
Expand Down
28 changes: 15 additions & 13 deletions WebServices/Controllers/AttributeSaveController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,24 +169,26 @@ private function ValidateRequest($request)
$errors[] = 'label is required';
}

if ($request->type != CustomAttributeTypes::CHECKBOX &&
$request->type != CustomAttributeTypes::MULTI_LINE_TEXTBOX &&
$request->type != CustomAttributeTypes::SELECT_LIST &&
$request->type != CustomAttributeTypes::SINGLE_LINE_TEXTBOX
if ((int)$request->type !== CustomAttributeTypes::CHECKBOX &&
(int)$request->type !== CustomAttributeTypes::MULTI_LINE_TEXTBOX &&
(int)$request->type !== CustomAttributeTypes::SELECT_LIST &&
(int)$request->type !== CustomAttributeTypes::SINGLE_LINE_TEXTBOX &&
(int)$request->type !== CustomAttributeTypes::DATETIME
Comment on lines +172 to +176
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

ValidateRequest() now treats CustomAttributeTypes::DATETIME as a valid type. There are existing unit tests for AttributeSaveController, but none assert that DATETIME requests pass validation (and/or that invalid types are rejected). Add a focused test case to cover this new allowed value so regressions are caught.

Copilot uses AI. Check for mistakes.
) {
$errors[] = sprintf(
'type is invalid. Allowed values for type: %s (checkbox), %s (multi line), %s (select list), %s (single line)',
'type is invalid. Allowed values for type: %s (checkbox), %s (multi line), %s (select list), %s (single line), %s (datetime)',
CustomAttributeTypes::CHECKBOX,
CustomAttributeTypes::MULTI_LINE_TEXTBOX,
CustomAttributeTypes::SELECT_LIST,
CustomAttributeTypes::SINGLE_LINE_TEXTBOX
CustomAttributeTypes::SINGLE_LINE_TEXTBOX,
CustomAttributeTypes::DATETIME
);
}

if ($request->categoryId != CustomAttributeCategory::RESERVATION &&
$request->categoryId != CustomAttributeCategory::RESOURCE &&
$request->categoryId != CustomAttributeCategory::RESOURCE_TYPE &&
$request->categoryId != CustomAttributeCategory::USER
if ((int)$request->categoryId !== CustomAttributeCategory::RESERVATION &&
(int)$request->categoryId !== CustomAttributeCategory::RESOURCE &&
(int)$request->categoryId !== CustomAttributeCategory::RESOURCE_TYPE &&
(int)$request->categoryId !== CustomAttributeCategory::USER
) {
$errors[] = sprintf(
'categoryId is invalid. Allowed values for category: %s (reservation), %s (resource), %s (resource type), %s (user)',
Expand All @@ -197,15 +199,15 @@ private function ValidateRequest($request)
);
}

if ($request->type == CustomAttributeTypes::SELECT_LIST && empty($request->possibleValues)) {
if ((int)$request->type === CustomAttributeTypes::SELECT_LIST && empty($request->possibleValues)) {
$errors[] = 'possibleValues is required when the type is a select list';
}

if ($request->type != CustomAttributeTypes::SELECT_LIST && !empty($request->possibleValues)) {
if ((int)$request->type !== CustomAttributeTypes::SELECT_LIST && !empty($request->possibleValues)) {
$errors[] = 'possibleValues is only valid when the type is a select list';
}

if ($request->categoryId == CustomAttributeCategory::RESERVATION && !empty($request->appliesToIds)) {
if ((int)$request->categoryId === CustomAttributeCategory::RESERVATION && !empty($request->appliesToIds)) {
Comment on lines 172 to 210
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

ValidateRequest() repeatedly casts $request->type/$request->categoryId inline. Consider casting once into local $type / $categoryId variables and reusing them (and optionally passing the casted ints into CustomAttribute::Create/CustomAttribute), to reduce duplication and keep types consistent after validation.

Copilot uses AI. Check for mistakes.
$errors[] = 'appliesToId is not valid when the type is reservation';
}

Expand Down
8 changes: 4 additions & 4 deletions lib/Application/Attributes/AttributeService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,9 +231,9 @@ public function GetReservationAttributes(UserSession $userSession, ReservationVi
$secondaryCategory = $attribute->SecondaryCategory();
if (empty($secondaryCategory) ||
(
$secondaryCategory == CustomAttributeCategory::USER &&
(int)$secondaryCategory === CustomAttributeCategory::USER &&
$this->AvailableForUser($userSession, $requestedUserId, $secondaryCategory, $attribute) ||
(($secondaryCategory == CustomAttributeCategory::RESOURCE || $secondaryCategory == CustomAttributeCategory::RESOURCE_TYPE)
(((int)$secondaryCategory === CustomAttributeCategory::RESOURCE || (int)$secondaryCategory === CustomAttributeCategory::RESOURCE_TYPE)
&& $this->AvailableForResource($userSession, $secondaryCategory, $attribute, $requestedResourceIds))
)
) {
Expand Down Expand Up @@ -276,8 +276,8 @@ private function AvailableForUser(UserSession $userSession, $requestedUserId, $s
*/
private function AvailableForResource($userSession, $secondaryCategory, $attribute, $requestedResourceIds)
{
if ($secondaryCategory == CustomAttributeCategory::RESOURCE || $secondaryCategory == CustomAttributeCategory::RESOURCE_TYPE) {
if ($secondaryCategory == CustomAttributeCategory::RESOURCE) {
if ((int)$secondaryCategory === CustomAttributeCategory::RESOURCE || (int)$secondaryCategory === CustomAttributeCategory::RESOURCE_TYPE) {
if ((int)$secondaryCategory === CustomAttributeCategory::RESOURCE) {
$applies = array_intersect($attribute->SecondaryEntityIds(), $requestedResourceIds);
Comment on lines +279 to 281
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

Within AvailableForResource(), you updated the initial category checks to strict comparisons, but the subsequent RESOURCE_TYPE branch still uses a loose == comparison. For consistency and to avoid mixed-type comparison warnings, make the RESOURCE_TYPE check strict as well (casting once if needed).

Copilot uses AI. Check for mistakes.
$allowed = array_intersect($attribute->SecondaryEntityIds(), array_keys($this->GetAllowedResources($userSession)));

Expand Down
6 changes: 3 additions & 3 deletions lib/Application/Reservation/Validation/CustomAttributeValidationRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,15 @@ public function Validate($reservationSeries, $retryParameters)
$secondaryCategory = $invalidAttribute->Attribute->SecondaryCategory();
$secondaryEntityIds = $invalidAttribute->Attribute->SecondaryEntityIds();

if ($secondaryCategory == CustomAttributeCategory::USER && !in_array($reservationSeries->UserId(), $secondaryEntityIds)) {
if ((int)$secondaryCategory === CustomAttributeCategory::USER && !in_array($reservationSeries->UserId(), $secondaryEntityIds)) {
// the attribute applies to a different user
continue;
}
if ($secondaryCategory == CustomAttributeCategory::RESOURCE && count(array_intersect($secondaryEntityIds, $reservationSeries->AllResourceIds())) == 0) {
if ((int)$secondaryCategory === CustomAttributeCategory::RESOURCE && count(array_intersect($secondaryEntityIds, $reservationSeries->AllResourceIds())) == 0) {
// the attribute is not for a resource that is being booked
continue;
}
if ($secondaryCategory == CustomAttributeCategory::RESOURCE_TYPE) {
if ((int)$secondaryCategory === CustomAttributeCategory::RESOURCE_TYPE) {
$appliesToResourceType = false;
foreach ($reservationSeries->AllResources() as $resource) {
if ($appliesToResourceType) {
Expand Down
8 changes: 4 additions & 4 deletions lib/Application/Schedule/ScheduleResourceFilter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,9 @@ public function FilterResources($resources, IResourceRepository $resourceReposit
}

/**
* @param Attribute[] $attributes
* @param LBAttribute[] $attributes
* @param int $attributeId
* @return null|Attribute
* @return null|LBAttribute
*/
private function GetAttribute($attributes, $attributeId)
{
Expand All @@ -176,7 +176,7 @@ private function GetAttribute($attributes, $attributeId)

/**
* @param AttributeValue $attribute
* @param Attribute $value
* @param LBAttribute $value
* @return bool
*/
private function AttributeValueMatches($attribute, $value)
Expand All @@ -185,7 +185,7 @@ private function AttributeValueMatches($attribute, $value)
return false;
}
Comment on lines 178 to 186
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

In AttributeValueMatches, the PHPDoc says $value is LBAttribute, but the implementation explicitly allows null (if ($value == null)). Update the docblock to LBAttribute|null (and consider using strict null check) so static analysis matches actual behavior.

Copilot uses AI. Check for mistakes.

if ($value->Type() == CustomAttributeTypes::SINGLE_LINE_TEXTBOX || $value->Type() == CustomAttributeTypes::MULTI_LINE_TEXTBOX) {
if ((int)$value->Type() == CustomAttributeTypes::SINGLE_LINE_TEXTBOX || (int)$value->Type() == CustomAttributeTypes::MULTI_LINE_TEXTBOX) {
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

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

This comparison still uses == even after casting to int. To fully address “strict equality” (and avoid phpstan mixed-type warnings), use === for these type comparisons (or assign the casted type to a local variable once and compare strictly).

Suggested change
if ((int)$value->Type() == CustomAttributeTypes::SINGLE_LINE_TEXTBOX || (int)$value->Type() == CustomAttributeTypes::MULTI_LINE_TEXTBOX) {
$type = (int)$value->Type();
if ($type === CustomAttributeTypes::SINGLE_LINE_TEXTBOX || $type === CustomAttributeTypes::MULTI_LINE_TEXTBOX) {

Copilot uses AI. Check for mistakes.
return strripos($value->Value() ?? "", $attribute->Value) !== false;
} elseif (is_numeric($value->Value())) {
return floatval($value->Value()) == $attribute->Value;
Expand Down
18 changes: 0 additions & 18 deletions phpstan-baseline.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,24 +96,6 @@ parameters:
count: 1
path: lib/Application/Schedule/ReservationService.php

-
message: '#^Call to an undefined method Attribute\:\:Id\(\)\.$#'
identifier: method.notFound
count: 1
path: lib/Application/Schedule/ScheduleResourceFilter.php

-
message: '#^Call to an undefined method Attribute\:\:Type\(\)\.$#'
identifier: method.notFound
count: 2
path: lib/Application/Schedule/ScheduleResourceFilter.php

-
message: '#^Call to an undefined method Attribute\:\:Value\(\)\.$#'
identifier: method.notFound
count: 4
path: lib/Application/Schedule/ScheduleResourceFilter.php

-
message: '#^PHPDoc tag @return with type Server is not subtype of native type IRestServer\|null\.$#'
identifier: return.phpDocType
Expand Down