chore(ZMSKVR-690 GH-1280): Upgrade from php 8.3 to 8.4 #1369
Conversation
…epare Github Actions, Composer Files, Docker Files
…s. Resolves PHP 8.4 deprecation: 'Implicitly marking parameter as nullable is deprecated, the explicit nullable type must be used instead'
…. Resolves remaining PHP 8.4 deprecation warnings about implicit nullable types
…Ensures comprehensive PHP 8.4 compatibility across all core classes
…Ensures comprehensive PHP 8.4 compatibility across all core classes
…lves PHP 8.4 compatibility issue.
…ents PHP 8.4 warning: 'Trying to access array offset on false'
…ue. Update isVerifiedHash method to use correct bcrypt cost parameter (y$). PHP 8.4 changed default bcrypt cost from 10 to 12. Fixes failing testCaptcha test in zmsentities module. Ensures hash generation and verification use same cost parameter.
WalkthroughProject-wide upgrade to PHP 8.4 (CI, images, DDEV, composer). Numerous functions and constructors made explicitly nullable; CSV exporter callback/BOM tweaks; Apikey bcrypt verification altered and captcha accessed as array; dependency/version tweaks, test adjustments, CLI module reordering, and minor build/config edits. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Apikey as Zmsentities::Apikey
Note over Apikey: isVerifiedHash now decodes stored value\nand builds bcrypt string with cost=12
Caller->>Apikey: isVerifiedHash($input, $storedHash)
Apikey->>Apikey: decoded = base64_decode($storedHash)
Apikey->>Apikey: bcryptString = "$2y$12$" + decoded
Apikey-->>Caller: return password_verify($input, bcryptString)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
- Fix offsetSet return type compatibility in zmsdldb Entity Base class - Add missing parameter in zmsdb Slot writeByAvailability call - Remove custom slim/psr7 repository from zmsticketprinter to resolve dependency conflicts
…n RequestException
…sage in XLS export
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
zmsdldb/src/Zmsdldb/Importer/MySQL/Entity/Base.php (1)
304-315: Update ArrayAccess implementations to usevoidreturn types foroffsetSet/offsetUnsetThe
\ArrayAccessinterface requires both methods to returnvoidin PHP 8.4. Currently we have three non-compliant signatures:• zmsdldb/src/Zmsdldb/Importer/MySQL/Entity/Base.php
• zmsdldb/src/Zmsdldb/Importer/MySQL/Entity/Collection.php
• zmsdldb/src/Zmsdldb/Entity/Base.phpApply the following changes:
--- a/zmsdldb/src/Zmsdldb/Importer/MySQL/Entity/Base.php +++ b/zmsdldb/src/Zmsdldb/Importer/MySQL/Entity/Base.php @@ - final public function offsetSet($offset, $value): Base + final public function offsetSet($offset, $value): void { $this->__set($offset, $value); - return $this; } @@ - final public function offsetUnset($offset): Base + final public function offsetUnset($offset): void { $this->__unset($offset); - return $this; }--- a/zmsdldb/src/Zmsdldb/Importer/MySQL/Entity/Collection.php +++ b/zmsdldb/src/Zmsdldb/Importer/MySQL/Entity/Collection.php @@ - final public function offsetSet($offset, $value): Collection + final public function offsetSet($offset, $value): void { if (!$value instanceof Base) { throw new InvalidArgumentException(...); - return $this; } @@ - final public function offsetUnset($offset): Collection + final public function offsetUnset($offset): void { unset($this->entities[$offset]); - return $this; }--- a/zmsdldb/src/Zmsdldb/Entity/Base.php +++ b/zmsdldb/src/Zmsdldb/Entity/Base.php @@ - public function offsetSet($index, $value): bool + public function offsetSet($index, $value): void { if ('data_json' == $index) { // ... } - return true; }After these changes, all
offsetSet/offsetUnsetmethods will comply with thevoidreturn type required by PHP 8.4’s\ArrayAccess.zmsdb/src/Zmsdb/Base.php (1)
44-48: Align init() signature to nullable PDOs as wellinit() still uses non-nullable PDO types with default nulls, which is exactly what PHP 8.4 deprecates. Make it explicitly nullable to match the constructor and avoid deprecation notices.
Apply:
- public static function init(\PDO $writeConnection = null, \PDO $readConnection = null) + public static function init(?\PDO $writeConnection = null, ?\PDO $readConnection = null)zmsdb/src/Zmsdb/Process.php (1)
205-208: Bug: wrong class constant reference (fatal at runtime)ENTITY::STATUS_CONFIRMED should be Entity::STATUS_CONFIRMED (case-sensitive class name). This will fatal if executed.
Apply:
- $status = ($keepReserved) ? Entity::STATUS_RESERVED : ENTITY::STATUS_CONFIRMED; + $status = ($keepReserved) ? Entity::STATUS_RESERVED : Entity::STATUS_CONFIRMED;zmsentities/src/Zmsentities/Validator/ProcessValidator.php (1)
74-111: Nullable callback accepted; remove commented debug with error_logSignature change is good. The commented-out debug block includes error_log and should be removed per logging guidelines.
Apply:
- /* - error_log( - "Mail validate: ".$valid->getUnvalidated() - ." ($length) with scope mail required=" - . ($process->getCurrentScope()->isEmailRequired() ? 'yes' : 'no') - ." with appointment=" - . ($process->isWithAppointment() ? 'yes' : 'no') - ." with callback=" - . ( ($isRequiredCallback && $isRequiredCallback()) ? 'yes' : 'no') - ); - */zmsdb/src/Zmsdb/Slot.php (1)
240-251: Critical: writeByAvailability signature changed—update all two-arg calls to include $slotLastChangeThe
writeByAvailability(Availability $availability, DateTimeInterface $now, DateTimeInterface $slotLastChange)method now requires three parameters. Any invocation with only two will trigger a runtime error. Affected call sites:
- zmsdb/src/Zmsdb/Slot.php → writeByScope(), line 249
- zmsapi/src/Zmsapi/AvailabilitySlotsUpdate.php → lines 73 and 84
- zmsdb/tests/Zmsdb/SlotTest.php → lines 279, 289, 318, 326, 332, 466, 508
You’ll need to compute or retrieve
$slotLastChange(e.g. viareadLastChangedTimeByAvailability()) and pass it as the third argument. For example, inwriteByScope():--- a/zmsdb/src/Zmsdb/Slot.php +++ b/zmsdb/src/Zmsdb/Slot.php @@ public function writeByScope(\BO\Zmsentities\Scope $scope, \DateTimeInterface $now) - if ($this->writeByAvailability($availability, $now)) { + if ($this->writeByAvailability($availability, $now, $slotLastChange)) { $updatedList->addEntity($availability); }Similarly update:
AvailabilitySlotsUpdate.php
// before (new SlotRepository())->writeByAvailability($availability, \App::$now); // after $slotLastChange = (new SlotRepository())->readLastChangedTimeByAvailability($availability); (new SlotRepository())->writeByAvailability($availability, \App::$now, $slotLastChange);SlotTest.php
$slotLastChange = (new Slot())->readLastChangedTimeByAvailability($availability); $status = (new Slot())->writeByAvailability($availability, $now, $slotLastChange);Ensure every two-arg call to
writeByAvailabilityis updated accordingly.
🧹 Nitpick comments (32)
zmsslim/composer.json (1)
12-12: Consider constraining to PHP 8.x explicitlyUsing ">=8.4" will also allow PHP 9.x when it releases, which may introduce breaking changes. Prefer an upper bound for predictability in CI/CD and local dev.
Apply this diff to be explicit:
- "php": ">=8.4", + "php": ">=8.4 <9.0",Optionally, set composer platform to enforce 8.4 during builds for reproducibility (can be done in the root composer.json):
"config": { "platform": { "php": "8.4.0" } }zmsadmin/composer.json (1)
37-37: Add an upper bound to the PHP requirementTo avoid unintended adoption of PHP 9 when it arrives, constrain to 8.x explicitly.
- "php": ">=8.4", + "php": ">=8.4 <9.0",If you prefer caret semantics instead:
- "php": ">=8.4", + "php": "^8.4",mellon/composer.json (1)
18-18: Tighten PHP version constraint to 8.x">=8.4" will accept future 9.x. Consider bounding to 8.x for safer upgrades.
- "php": ">=8.4", + "php": ">=8.4 <9.0",zmsapi/composer.json (1)
35-35: Constrain PHP requirement to the intended majorTo prevent accidental adoption of PHP 9, bound the range to 8.x.
- "php": ">=8.4", + "php": ">=8.4 <9.0",If desired, I can open a follow-up PR to apply this consistently across all modules and add a composer platform pin at the repository root.
zmsclient/composer.json (1)
26-26: Prefer explicit upper bound for PHPKeep the requirement within 8.x to avoid future 9.x surprises.
- "php": ">=8.4", + "php": ">=8.4 <9.0",.ddev/config.yaml (1)
4-6: DDEV php_version updated to 8.4. LGTM.This keeps local dev aligned with CI and composer constraints. Minor nit: the long commented help block still lists versions up to 8.3 (upstream template). No action needed unless it confuses the team.
zmsdb/tests/Zmsdb/ClusterTest.php (1)
137-137: Prefer asserting the expected entity type over a null-checkAsserting the specific entity class aligns with other tests in this file and yields a stronger guarantee than non-null.
Apply this diff for consistency:
- $this->assertNotNull($entity, 'writeEntity should not return null'); + $this->assertEntity("\\BO\\Zmsentities\\Cluster", $entity);.github/workflows/php-code-quality.yaml (1)
37-61: PHP 8.4 bump across modules: looks good; add newline at EOFThe version bump aligns with the rest of the repo. YAML lint flagged a missing newline at end of file; please add one to satisfy linters.
zmsdldb/src/Zmsdldb/Entity/Base.php (1)
98-105: Fix stripos checks (0 is falsy) to avoid missing matches at position 0
stripos(...)returns 0 for prefix matches, which is falsy, so the condition silently fails. Use a strict comparison.See the diff above where both conditions are changed to
false !== stripos(...).zmsentities/src/Zmsentities/Process.php (1)
434-438: Optional: align setCallTime signature with setArrivalTimeFor consistency and clearer contracts, consider updating
setCallTime($dateTime = null)tosetCallTime(?\DateTimeInterface $dateTime = null).- public function setCallTime($dateTime = null) + public function setCallTime(?\DateTimeInterface $dateTime = null) { $this->queue['callTime'] = ($dateTime) ? $dateTime->getTimestamp() : 0; return $this; }zmsslim/src/Slim/Response.php (1)
25-25: Tighten signature: union type for $url and static return for accuracyMinor type improvements will make the contract clearer and align with the docblock that claims “@return static”.
Apply this diff on the method signature:
- public function withRedirect($url, ?int $status = null): Response + public function withRedirect(string|UriInterface $url, ?int $status = null): staticOptionally, adjust the docblock to remove ambiguity if you don’t switch to static:
@return Responsezmsentities/src/Zmsentities/Apikey.php (1)
53-57: Array-access vs property access: be consistent within the entitywithCaptchaData now uses array-access ($this['captcha']), while other members (e.g., quota at Lines 61-81) use property access ($this->quota). Mixed access works if Schema\Entity supports both, but it increases cognitive load.
Consider standardizing on a single access pattern across this class (preferably array-access if that’s the direction elsewhere).
.github/workflows/php-unit-tests.yaml (1)
89-89: Trim trailing whitespaceYamllint flags trailing spaces here.
Apply this diff:
- php_version: "8.4" + php_version: "8.4"zmsclient/src/Zmsclient/Exception.php (1)
53-56: Prefer ?Throwable for $previous and align docblocks with nullabilityThe nullable updates are aligned with PHP 8.x. For $previous, consider widening to ?Throwable to allow chaining Errors as previous exceptions (base Exception::__construct accepts ?Throwable). Also, property docblocks for $response and $request still lack “|null”.
Apply:
- ?\Exception $previous = null + ?\Throwable $previous = nullAnd update docblocks (outside this hunk):
- * @var \Psr\Http\Message\ResponseInterface $response + * @var \Psr\Http\Message\ResponseInterface|null $response ... - * @var \Psr\Http\Message\RequestInterface $request + * @var \Psr\Http\Message\RequestInterface|null $requestzmsclient/src/Zmsclient/Exception/ClientCreationException.php (1)
20-25: Signature improvement looks good; consider adding scalar types for consistencyMaking $previous nullable aligns with parent and repo-wide changes. As a small improvement, add scalar type hints for $message and $code now that you target PHP 8.4.
- public function __construct($message = "", $code = 0, ?Throwable $previous = null) + public function __construct(string $message = "", int $code = 0, ?Throwable $previous = null)zmsentities/src/Zmsentities/Helper/DateTime.php (1)
7-8: Nullable timezone is correct; consider return type and clearer $time typingThe nullability change matches usage. Optionally, make the contract explicit with a return type and a union for $time, which improves static analysis without changing behavior.
- public static function create($time = 'now', ?\DateTimeZone $timezone = null) + public static function create(\DateTimeInterface|string|self $time = 'now', ?\DateTimeZone $timezone = null): selfzmsdb/src/Zmsdb/Base.php (2)
38-42: Constructor: explicit nullable PDOs fix implicitly-nullable deprecationGood change. This removes the “implicitly nullable parameter” deprecation in newer PHP versions while preserving behavior.
For consistency, align related factory helpers (see next comment).
102-106: Remove unused argument passed to closure in pdoExceptionHandlerpdoExceptionHandler invokes the closure with a parameter, while all call sites use a zero-arg closure capturing variables via use(). Passing an extra argument is unnecessary and makes the signature brittle.
Apply:
- $statement = $pdoFunction($parameters); + $statement = $pdoFunction();zmsdb/src/Zmsdb/Availability.php (3)
62-64: Nullable DateTimeInterface parameters: OK, maintainers should confirm callersAllowing nulls here matches the downstream conditional logic. Ensure upstreams either pass both dates, or only startDate, to avoid surprising ranges.
Optional: document precedence in the phpdoc (what happens if only startDate is provided).
103-105: Mixed DateTime types: consider consistency across APIsreadAvailabilityListByScope now uses ?DateTimeImmutable while readList uses ?DateTimeInterface. If the underlying query methods accept either, prefer one consistently (ideally DateTimeImmutable) across both to simplify usage and reduce accidental mutation.
Would you like a small follow-up PR to standardize these signatures?
124-126: Convert TODO into a tracked issueThe inline TODO “check resolvedLevel from scope” represents technical debt. Please either address it or create a ticket and reference it here.
I can open a new issue and draft a checklist for verifying resolve levels across the scope resolution path.
zmsdb/src/Zmsdb/Process.php (2)
524-539: Use strict null-check for optional scopeIdCurrently using if ($scopeId) will skip 0, which may be fine if 0 is invalid, but given the type is ?int the clearer intent is to check for null explicitly.
Apply:
- if ($scopeId) { + if ($scopeId !== null) { $query->addConditionScopeId($scopeId); }
378-406: Add early validation and normalization of date parameters in readConflictListByScopeAndTimeAll existing callers supply at least one of $startDate or $now, but if both were ever null the loop’s comparisons would error. To future-proof this method, normalize and guard at the top of the date logic:
In zmsdb/src/Zmsdb/Process.php inside readConflictListByScopeAndTime() (lines 378–406), replace:
- if (! $endDate) { - $availabilityList = $availabilityList->withDateTime($startDate); - $endDate = $startDate; - } - $currentDate = ($startDate) ? $startDate : $now;with:
+ $currentStart = $startDate ?? $now; + if ($currentStart === null) { + throw new \InvalidArgumentException('Either $startDate or $now must be provided'); + } + if ($endDate === null) { + $availabilityList = $availabilityList->withDateTime($currentStart); + $endDate = $currentStart; + } + $currentDate = $currentStart;zmsentities/src/Zmsentities/Validator/ProcessValidator.php (1)
134-169: Telephone validator: nullable callback change looks goodNo functional changes; the normalization via libphonenumber remains guarded. All good.
You may cache ($isRequiredCallback && $isRequiredCallback()) in a local variable to avoid potential repeated work/side effects of the callback.
zmsclient/src/Zmsclient/Http.php (4)
70-89: Constructor: nullable ClientInterface OK; add scalar type for $baseUrlAdd a scalar type for $baseUrl to tighten API and catch misuse early.
- public function __construct($baseUrl, ?ClientInterface $client = null) + public function __construct(string $baseUrl, ?ClientInterface $client = null)
175-188: readGetResult: nullable array is fine; type $xToken as nullable stringMinor type-safety improvement.
- public function readGetResult($relativeUrl, ?array $getParameters = null, $xToken = null) + public function readGetResult($relativeUrl, ?array $getParameters = null, ?string $xToken = null)
199-211: Ensure JSON requests set Content-Type headerBody is JSON-encoded but no explicit Content-Type is set. Some servers rely on it.
$request = self::createRequest('POST', $uri); $body = new Psr7\Stream(); $body->write(json_encode($entity)); $request = $request->withBody($body); + $request = $request->withHeader('Content-Type', 'application/json');
121-125: Avoid consuming the response body when logging sizegetContents() advances the stream pointer; downstream consumers may see an empty body. Prefer getSize() or preserve/restore the pointer when measuring.
- $responseSizeKb = round(strlen($response->getBody()->getContents()) / 1024); + $stream = $response->getBody(); + $size = $stream->getSize(); + $responseSizeKb = ($size !== null) ? round($size / 1024) : null; + if ($responseSizeKb === null && $stream->isSeekable()) { + $pos = $stream->tell(); + $stream->rewind(); + $responseSizeKb = round(strlen($stream->getContents()) / 1024); + $stream->seek($pos); + }zmsdb/src/Zmsdb/Slot.php (4)
179-186: Remove dead fallback for $slotLastChange (now non-null)With a non-nullable parameter, this conditional is never reached. Clean it up to reduce confusion.
- if (!$slotLastChange) { - $slotLastChange = $this->readLastChangedTimeByAvailability($availability); - }
378-394: Prefer explicit null-check for scopeIDWith ?int $scopeID, use strict null-check to avoid accidental falsey matches if 0 ever appears.
- if ($scopeID) { + if ($scopeID !== null) {
396-407: Prefer explicit null-check for scopeIDMirror the nullability intent consistently.
- if ($scopeID) { + if ($scopeID !== null) {
409-425: Prefer explicit null-check for scopeIDConsistent handling across the API.
- if ($scopeID) { + if ($scopeID !== null) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (13)
mellon/composer.lockis excluded by!**/*.lockzmsadmin/composer.lockis excluded by!**/*.lockzmsapi/composer.lockis excluded by!**/*.lockzmscalldisplay/composer.lockis excluded by!**/*.lockzmscitizenapi/composer.lockis excluded by!**/*.lockzmsclient/composer.lockis excluded by!**/*.lockzmsdb/composer.lockis excluded by!**/*.lockzmsdldb/composer.lockis excluded by!**/*.lockzmsentities/composer.lockis excluded by!**/*.lockzmsmessaging/composer.lockis excluded by!**/*.lockzmsslim/composer.lockis excluded by!**/*.lockzmsstatistic/composer.lockis excluded by!**/*.lockzmsticketprinter/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.ddev/config.yaml(1 hunks).github/workflows/build-api-docs.yaml(1 hunks).github/workflows/owasp-security-checks.yaml(1 hunks).github/workflows/php-build-images.yaml(1 hunks).github/workflows/php-code-quality.yaml(1 hunks).github/workflows/php-unit-tests.yaml(3 hunks)mellon/composer.json(1 hunks)zmsadmin/composer.json(1 hunks)zmsadmin/src/Zmsadmin/PickupSpreadSheet.php(1 hunks)zmsadmin/src/Zmsadmin/ScopeAppointmentsByDayXlsExport.php(1 hunks)zmsadmin/src/Zmsadmin/WorkstationProcessFinished.php(1 hunks)zmsapi/composer.json(1 hunks)zmscalldisplay/composer.json(1 hunks)zmscitizenapi/composer.json(1 hunks)zmscitizenapi/src/Zmscitizenapi/Services/Captcha/CaptchaService.php(0 hunks)zmsclient/composer.json(2 hunks)zmsclient/docker-compose.yml(1 hunks)zmsclient/src/Zmsclient/Exception.php(1 hunks)zmsclient/src/Zmsclient/Exception/ClientCreationException.php(1 hunks)zmsclient/src/Zmsclient/Http.php(5 hunks)zmsclient/src/Zmsclient/Result.php(1 hunks)zmsdb/composer.json(2 hunks)zmsdb/src/Zmsdb/Availability.php(2 hunks)zmsdb/src/Zmsdb/Base.php(2 hunks)zmsdb/src/Zmsdb/Process.php(3 hunks)zmsdb/src/Zmsdb/Query/Builder/Query.php(3 hunks)zmsdb/src/Zmsdb/Query/Builder/Values.php(1 hunks)zmsdb/src/Zmsdb/Slot.php(6 hunks)zmsdb/src/Zmsdb/Source/Zmsdldb.php(1 hunks)zmsdb/tests/Zmsdb/ClusterTest.php(1 hunks)zmsdldb/composer.json(1 hunks)zmsdldb/src/Zmsdldb/Entity/Base.php(1 hunks)zmsdldb/src/Zmsdldb/Importer/MySQL/Entity/Base.php(1 hunks)zmsentities/composer.json(1 hunks)zmsentities/src/Zmsentities/Apikey.php(1 hunks)zmsentities/src/Zmsentities/Helper/DateTime.php(1 hunks)zmsentities/src/Zmsentities/Process.php(1 hunks)zmsentities/src/Zmsentities/Validator/ProcessValidator.php(4 hunks)zmsentities/tests/Zmsentities/ApikeyTest.php(1 hunks)zmsmessaging/composer.json(1 hunks)zmsslim/composer.json(2 hunks)zmsslim/src/Slim/Response.php(1 hunks)zmsstatistic/composer.json(1 hunks)zmsticketprinter/composer.json(1 hunks)zmsticketprinter/src/Zmsticketprinter/Helper/QueueListHelper.php(1 hunks)
💤 Files with no reviewable changes (1)
- zmscitizenapi/src/Zmscitizenapi/Services/Captcha/CaptchaService.php
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit Configuration File
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsclient/composer.jsonzmsslim/composer.jsonzmsdb/tests/Zmsdb/ClusterTest.phpmellon/composer.jsonzmsstatistic/composer.jsonzmsadmin/src/Zmsadmin/PickupSpreadSheet.phpzmsdldb/composer.jsonzmsentities/src/Zmsentities/Process.phpzmsadmin/composer.jsonzmscalldisplay/composer.jsonzmsentities/composer.jsonzmsmessaging/composer.jsonzmsdb/src/Zmsdb/Query/Builder/Values.phpzmsadmin/src/Zmsadmin/WorkstationProcessFinished.phpzmscitizenapi/composer.jsonzmsclient/src/Zmsclient/Exception/ClientCreationException.phpzmsentities/tests/Zmsentities/ApikeyTest.phpzmsclient/docker-compose.ymlzmsclient/src/Zmsclient/Result.phpzmsentities/src/Zmsentities/Apikey.phpzmsadmin/src/Zmsadmin/ScopeAppointmentsByDayXlsExport.phpzmsslim/src/Slim/Response.phpzmsapi/composer.jsonzmsdb/composer.jsonzmsdb/src/Zmsdb/Query/Builder/Query.phpzmsentities/src/Zmsentities/Helper/DateTime.phpzmsticketprinter/composer.jsonzmsdldb/src/Zmsdldb/Importer/MySQL/Entity/Base.phpzmsdb/src/Zmsdb/Process.phpzmsdb/src/Zmsdb/Source/Zmsdldb.phpzmsdldb/src/Zmsdldb/Entity/Base.phpzmsticketprinter/src/Zmsticketprinter/Helper/QueueListHelper.phpzmsentities/src/Zmsentities/Validator/ProcessValidator.phpzmsdb/src/Zmsdb/Base.phpzmsdb/src/Zmsdb/Availability.phpzmsclient/src/Zmsclient/Http.phpzmsclient/src/Zmsclient/Exception.phpzmsdb/src/Zmsdb/Slot.php
**/*.php
⚙️ CodeRabbit Configuration File
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/tests/Zmsdb/ClusterTest.phpzmsadmin/src/Zmsadmin/PickupSpreadSheet.phpzmsentities/src/Zmsentities/Process.phpzmsdb/src/Zmsdb/Query/Builder/Values.phpzmsadmin/src/Zmsadmin/WorkstationProcessFinished.phpzmsclient/src/Zmsclient/Exception/ClientCreationException.phpzmsentities/tests/Zmsentities/ApikeyTest.phpzmsclient/src/Zmsclient/Result.phpzmsentities/src/Zmsentities/Apikey.phpzmsadmin/src/Zmsadmin/ScopeAppointmentsByDayXlsExport.phpzmsslim/src/Slim/Response.phpzmsdb/src/Zmsdb/Query/Builder/Query.phpzmsentities/src/Zmsentities/Helper/DateTime.phpzmsdldb/src/Zmsdldb/Importer/MySQL/Entity/Base.phpzmsdb/src/Zmsdb/Process.phpzmsdb/src/Zmsdb/Source/Zmsdldb.phpzmsdldb/src/Zmsdldb/Entity/Base.phpzmsticketprinter/src/Zmsticketprinter/Helper/QueueListHelper.phpzmsentities/src/Zmsentities/Validator/ProcessValidator.phpzmsdb/src/Zmsdb/Base.phpzmsdb/src/Zmsdb/Availability.phpzmsclient/src/Zmsclient/Http.phpzmsclient/src/Zmsclient/Exception.phpzmsdb/src/Zmsdb/Slot.php
🧬 Code Graph Analysis (9)
zmsadmin/src/Zmsadmin/WorkstationProcessFinished.php (1)
zmsentities/src/Zmsentities/Schema/Entity.php (1)
Entity(12-372)
zmsclient/src/Zmsclient/Exception/ClientCreationException.php (1)
zmsdb/src/Zmsdb/Base.php (1)
__construct(38-42)
zmsentities/src/Zmsentities/Apikey.php (1)
zmsentities/src/Zmsentities/Mimepart.php (1)
Mimepart(5-48)
zmsdb/src/Zmsdb/Query/Builder/Query.php (1)
zmsdb/src/Zmsdb/Query/Base.php (1)
__construct(88-121)
zmsdb/src/Zmsdb/Process.php (2)
zmsentities/src/Zmsentities/Scope.php (1)
Scope(11-375)zmsdb/src/Zmsdb/Useraccount.php (1)
Useraccount(12-305)
zmsdldb/src/Zmsdldb/Entity/Base.php (1)
zmsentities/src/Zmsentities/Schema/Entity.php (1)
exchangeArray(63-69)
zmsticketprinter/src/Zmsticketprinter/Helper/QueueListHelper.php (2)
zmsclient/src/Zmsclient/Http.php (1)
__construct(70-89)zmsdb/src/Zmsdb/Base.php (1)
__construct(38-42)
zmsentities/src/Zmsentities/Validator/ProcessValidator.php (1)
mellon/src/Mellon/Unvalidated.php (1)
Unvalidated(15-81)
zmsdb/src/Zmsdb/Base.php (3)
zmsclient/src/Zmsclient/Http.php (1)
__construct(70-89)zmsclient/src/Zmsclient/Exception/ClientCreationException.php (1)
__construct(20-25)zmsticketprinter/src/Zmsticketprinter/Helper/QueueListHelper.php (1)
__construct(23-27)
🪛 YAMLlint (1.37.1)
.github/workflows/php-code-quality.yaml
[error] 61-61: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/php-unit-tests.yaml
[error] 89-89: trailing spaces
(trailing-spaces)
🪛 actionlint (1.7.7)
.github/workflows/php-unit-tests.yaml
257-257: input "php_version" is not defined in action "shivammathur/setup-php@v2". available inputs are "coverage", "extensions", "ini-file", "ini-values", "php-version", "php-version-file", "tools"
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsclient-test
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (37)
zmsclient/composer.json (1)
37-37: PSR-7 version validated: 1.7.1 is the latest 1.x release
Confirmed via Packagist and composer.lock that no 1.x beyond 1.7.1 exists. Your constraint "^1.7.1" will pull in the current patch and any future 1.x fixes. If you haven’t yet, skim the changelog for any stream or uploaded-file behavior changes between 1.7.0 and 1.7.1 to ensure nothing breaks your integration.zmscitizenapi/composer.json (1)
28-39: All PHP constraints and CI/DDEV settings consistently use >=8.4Verification confirms:
- Every module’s composer.json (
mellon,zmsadmin,zmsapi, …) now requires PHP >=8.4- GitHub Actions (
build-api-docs.yaml) is pinned tophp-version: '8.4'- DDEV config (
.ddev/config.yaml) specifiesphp_version: "8.4"LGTM—approving these changes.
zmsentities/composer.json (1)
11-26: Consistent bump to PHP >=8.4. Looks good.Matches the global upgrade and should not introduce resolver issues.
zmsmessaging/composer.json (1)
19-33: PHP constraint bumped to >=8.4. LGTM.No concerns from the dependency set in this file with 8.4.
zmsstatistic/composer.json (1)
36-51: PHP >=8.4 requirement is correct and consistent.No other changes here; dependency ranges remain compatible.
zmsdb/composer.json (2)
38-38: Verify ExtendedPdo & Profiler compatibility with Aura SQL v6We’ve updated zmsdb/composer.json to
"aura/sql": "^6.0". Please confirm that these APIs remain unchanged in v6:• zmsdb/src/Zmsdb/Connection/Pdo.php
– class extends\Aura\Sql\ExtendedPdo
• zmsdb/src/Zmsdb/Connection/Select.php
– instantiates\Aura\Sql\Profiler\Profiler
– callssetActive()andsetProfiler()No other pins of
aura/sqlor references toaura/sqlquerywere found.
Review the Aura SQL v6 changelog for any BC breaks around ExtendedPdo or Profiler, and ensure your test suite/pass smoke tests.
26-26: PHP 8.4 upgrade consistency verifiedAll composer.json files now require PHP >=8.4, and no PHP 8.3 references remain in:
- Workflows (no
.github/workflowsYAML files containedphp-version: 8.3)- DDEV config (
.ddev/config.yamlhad nophp_version: 8.3)- Dockerfiles (no references to “8.3”)
zmscalldisplay/composer.json (1)
28-28: PHP >=8.4 constraint — LGTMMatches the broader migration. No further issues from this file.
.github/workflows/build-api-docs.yaml (1)
25-25: No additional PHP extensions required for PHP 8.4
I’ve verified bothzmsapi/composer.jsonandzmscitizenapi/composer.json—neither listsext-intl(or any otherext-*) as a dependency. The default setup-php environment for PHP 8.4 will satisfy all composer installs, so no change to the workflow is needed.zmsdldb/composer.json (1)
24-24: PHP >=8.4 constraint — LGTMAligned with the repo-wide upgrade. Dev deps (e.g., phpunit ^10.5) should be verified against 8.4 in CI, but no changes needed here.
zmsclient/docker-compose.yml (1)
17-17: PHP 8.4-dev base image bump for tests: LGTMConsistent with the repo-wide upgrade. No other service config changes; volumes/env remain compatible.
Please confirm that the ghcr.io/it-at-m/eappointment-php-base:8.4-dev image includes xdebug, since the command enables it via zend_extension=xdebug.so and relies on coverage.
.github/workflows/owasp-security-checks.yaml (1)
18-42: Matrix bumped to PHP 8.4 across all modules: LGTMThis keeps the OWASP checks aligned with the new runtime. Container image reference also matches 8.4 via the matrix.
Ensure the 8.4-dev base image tag exists in GHCR prior to running this workflow; otherwise jobs will fail on container start.
zmsdb/src/Zmsdb/Source/Zmsdldb.php (1)
58-60: Tighten topic guard to an explicit array check
Switching from a generic$topic !== falsetois_array($topic)makes the intent clear and preventsnull, empty strings or other falsy/truthy values from slipping through before accessing['name']. Please confirm thatfromTopic()->fetchId()only ever returnsarrayorfalse.Affected location:
- zmsdb/src/Zmsdb/Source/Zmsdldb.php (around lines 58–60)
Proposed change:
- if ($topic !== false && isset($topic['name'])) { + if (is_array($topic) && isset($topic['name'])) { $request['group'] = $topic['name']; }.github/workflows/php-build-images.yaml (1)
52-64: Build matrix upgraded to PHP 8.4: LGTMThis correctly propagates PHP_VERSION=8.4 to image builds.
Please verify your Containerfile accepts PHP_VERSION=8.4 and that any base images it references with that tag are already published to avoid build failures.
zmsadmin/src/Zmsadmin/PickupSpreadSheet.php (1)
50-56: League CSV v9.x APIs verified – no changes requiredBoth usages align with v9.x:
- Passing the escapeRecord method as a callable (e.g.
[$escapeFormula, 'escapeRecord']) is equivalent to the documented first-class callable syntax.setOutputBOM(\League\Csv\Bom::Utf8)correctly uses the newBomenum in v9.x.No updates needed here.
zmsdldb/src/Zmsdldb/Importer/MySQL/Entity/Base.php (1)
366-381: Nullable key parameter aligns with callers and is safeChanging
arrayAccessByDotPerpareKeysto accept?stringis consistent with theget()usage and avoids unnecessary casts/guards. Looks good.zmsentities/src/Zmsentities/Process.php (1)
629-633: LGTM: nullable DateTimeInterface is correct hereAllowing
nullforsetArrivalTimematches existing behavior (0 timestamp when absent) and aligns with broader nullable updates in the PR.zmsticketprinter/composer.json (1)
32-32: PHP >=8.4 compatibility and removed VCS entries verifiedAll checks passed—there are no residual references to PHP 8.0–8.3 or to the slim-psr7 fork/VCS URL in any
composer.json. This bump to PHP >=8.4 can be merged safely.
- No
composer.jsonfiles pinning PHP 8.0–8.3- No occurrences of
slim-psr7or the old GitLab VCS URL anywhere outsidevendor/zmsslim/src/Slim/Response.php (1)
25-25: Nullable type change looks good and PHP 8.4-readySwitching to a nullable int for $status is correct and BC-safe.
zmsentities/tests/Zmsentities/ApikeyTest.php (1)
30-30: LGTM: Test updated to new array-style captcha accessThe assertion now reflects the entity’s array-access for captcha data.
.github/workflows/php-unit-tests.yaml (1)
112-112: Container images updated to 8.4 look correctThe move to ghcr.io/it-at-m/eappointment-php-base:8.4-dev for zmsapi-test and zmsdb-test aligns with the matrix bump and the project-wide upgrade.
Also applies to: 182-182
zmsadmin/src/Zmsadmin/ScopeAppointmentsByDayXlsExport.php (2)
72-78: Verify delimiter: previous semicolon default removed?The writer no longer sets a delimiter; League\Csv defaults to comma. If consumers (Excel in DE locales) expect semicolons, this silently changes the file format.
If semicolon is still desired, add:
$writer = Writer::createFromString(); + $writer->setDelimiter(';');
77-77: Confirmed:\League\Csv\Bom::Utf8is correct for league/csv ^9.0
League\Csv v9+ introduced theBomenum withUtf8(camel-case) and deprecated the olderReader::BOM_UTF8andByteSequence::BOM_UTF8. Your existing call is valid—no changes needed.zmsclient/src/Zmsclient/Result.php (1)
43-49: Constructor nullability update is safe; all call sites pass both argumentsA scan of every
new Result(...)instantiation confirms that each call—including the one inHttp.php—passes both$responseand$request. No instantiations omit the second parameter.
- zmsclient/src/Zmsclient/Http.php:236 —
new Result($response, $request);zmsadmin/src/Zmsadmin/WorkstationProcessFinished.php (1)
73-79: Nullability update is correct and type safety confirmedAutomated inspection shows that
ProcessFinishedHelperextends\BO\Zmsentities\Process, so passing its instances into
getFinishedResponse(Workstation, ?Entity $process)is type-safe.No further changes required.
zmsticketprinter/src/Zmsticketprinter/Helper/QueueListHelper.php (1)
23-27: Tighten factory‐method signatures for QueueListHelperAll instantiations in the zmsticketprinter module pass the correct types:
- Process.php (line 49):
new QueueListHelper($scope, $process)
→$scopeis\BO\Zmsentities\Scope,$processis?\BO\Zmsentities\Process- TicketprinterByScope.php (line 43):
new QueueListHelper($scope)
→ omits the second param (defaults tonull)- NotificationAssign.php (line 70):
new QueueListHelper($process->getCurrentScope())
→ first arg is aScope, second is implicitlynullYou can safely tighten the internal factory methods to improve static analysis:
protected static function createFullList(\BO\Zmsentities\Scope $scope): QueueList protected static function createQueueList(?\BO\Zmsentities\Process $process): QueueListBe sure to import or fully qualify
QueueListas needed.zmsdb/src/Zmsdb/Base.php (1)
267-287: hashStringValue: nullable callable is correct; behavior preservedExplicitly allowing a nullable callable aligns with the default behavior of falling back to sha1. The callable classification logic remains intact.
zmsdb/src/Zmsdb/Process.php (1)
125-140: writeNewPickup: nullable Useraccount is fine; behavior unchangedThe change is safe and keeps downstream logging optional. No functional differences.
zmsentities/src/Zmsentities/Validator/ProcessValidator.php (2)
45-59: Nullable callback parameter: good modernizationAllowing a nullable isRequiredCallback matches how you already null-check before invocation. No behavior change.
61-72: Same here: OK to accept nullable callbackConsistent with the rest of the validator methods.
zmsclient/src/Zmsclient/Http.php (1)
221-229: LGTM: readDeleteResult signature change is consistentNullable GET parameters align with the broader 8.4 refactor.
zmsdb/src/Zmsdb/Slot.php (2)
75-76: LGTM: hasScopeRelevantChanges now requires non-null DateTimeInterfaceThe stricter signature prevents null dereferencing on $slotLastChange.
103-104: LGTM: isAvailabilityOutdated now requires non-null DateTimeInterfaceConsistent with upstream usage and internal comparisons.
zmsdb/src/Zmsdb/Query/Builder/Values.php (1)
24-32: LGTM: values() now explicitly allows nullable arraySignature matches the get-vs-set pattern and improves static analysis.
zmsdb/src/Zmsdb/Query/Builder/Query.php (3)
38-41: LGTM: constructor accepts nullable DialectInterfaceDefaults to ANSI dialect when null; behavior preserved.
65-72: LGTM: dialect() getter/setter with nullable paramMethod overloading-by-null is clear and consistent.
103-112: Verified: no non-array calls toflags()foundA search across the repository shows only these invocations in tests—either with no arguments or a variable that’s always an array:
• zmsdb/tests/Zmsdb/Query/Builder/QueryTest.php:
– Line 26:$query->flags()
– Lines 60–61:$query->flags($flags)No other call sites pass non-array values. Changing the signature to
?array $flags = nullis therefore backward-compatible.
| ?RequestInterface $request = null, | ||
| $try = 0 | ||
| ) { |
There was a problem hiding this comment.
Bug: readResult allows null request but immediately requires a non-null RequestInterface
readResult now accepts a nullable RequestInterface with a default null, but it immediately calls readResponse($request), which strictly requires a non-null RequestInterface. This will fatally error if readResult() is ever invoked without a request.
Recommend reverting to a non-nullable, required parameter (and optionally typing $try as int):
- protected function readResult(
- ?RequestInterface $request = null,
- $try = 0
- ) {
+ protected function readResult(
+ RequestInterface $request,
+ int $try = 0
+ ) {🤖 Prompt for AI Agents
In zmsclient/src/Zmsclient/Http.php around lines 232 to 234, the readResult
method currently declares ?RequestInterface $request = null but immediately
passes $request to readResponse which requires a non-null RequestInterface,
causing a fatal error if invoked without a request; change the signature to
require a non-null RequestInterface (remove the nullable and default) and
optionally type-hint $try as int, then update any callers to always pass a
RequestInterface instance (or add a guard/early-return in callers) so readResult
never receives null.
zmsdldb/src/Zmsdldb/Entity/Base.php
Outdated
| public function offsetSet($index, $value): bool | ||
| { | ||
| if ('data_json' == $index) { | ||
| $value = json_decode($value, true); | ||
| $this->exchangeArray($value); | ||
| return false; | ||
| } else { | ||
| if (stripos($index, '_json')) { | ||
| $value = json_decode($value, true); | ||
| $index = str_replace('_json', '', $index); | ||
| } | ||
| if (stripos($index, '__')) { | ||
| static::doubleUnterlineToArray($this, $index, $value); | ||
| return; | ||
| return true; | ||
| } | ||
|
|
||
| parent::offsetSet($index, $value); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Incompatible signature: offsetSet must return void (breaks ArrayObject in PHP 8.4)
Base extends \ArrayObject. Overriding offsetSet with : bool violates the parent signature (void) and will be fatal in PHP 8.4. Also, the method’s return value is unused by callers, so returning a boolean provides no value.
Apply:
- public function offsetSet($index, $value): bool
+ public function offsetSet($index, $value): void
{
if ('data_json' == $index) {
$value = json_decode($value, true);
$this->exchangeArray($value);
- return false;
+ return;
} else {
- if (stripos($index, '_json')) {
+ if (false !== stripos($index, '_json')) {
$value = json_decode($value, true);
$index = str_replace('_json', '', $index);
}
- if (stripos($index, '__')) {
+ if (false !== stripos($index, '__')) {
static::doubleUnterlineToArray($this, $index, $value);
- return true;
+ return;
}
parent::offsetSet($index, $value);
- return false;
+ return;
}
}🤖 Prompt for AI Agents
In zmsdldb/src/Zmsdldb/Entity/Base.php around lines 91 to 110, the overridden
offsetSet currently declares a boolean return type and returns true/false, which
conflicts with \ArrayObject::offsetSet(string|int|null $index, mixed $newval):
void in PHP 8.4; remove the ": bool" return type so the method returns void, and
replace all "return false" / "return true" with plain "return;" or no return at
all (ensure control flow still exits the method where intended), keeping the
existing behavior of decoding '_json' fields and handling double-underscore
keys.
| $decoded = base64_decode($hash); | ||
| $reconstructed = '$2y$12$' . $decoded; | ||
| return password_verify($text . $secret, $reconstructed); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential breakage: hardcoded bcrypt cost (12) may invalidate existing hashes
The verification reconstructs the bcrypt prefix using cost 12, but getHash() does not pin a cost (uses default). If historical hashes were generated with cost 10, verification will now fail. This is brittle and likely to break existing tokens/keys.
Recommended migration-safe verification: try both 12 and 10 (and fail fast on invalid base64). This preserves compatibility while allowing new hashes to move to cost 12.
- $decoded = base64_decode($hash);
- $reconstructed = '$2y$12$' . $decoded;
- return password_verify($text . $secret, $reconstructed);
+ $decoded = base64_decode($hash, true);
+ if ($decoded === false) {
+ return false;
+ }
+ foreach (['$2y$12$', '$2y$10$'] as $prefix) {
+ if (password_verify($text . $secret, $prefix . $decoded)) {
+ return true;
+ }
+ }
+ return false;Follow-up (optional but recommended):
- Pin the cost in getHash() too, and store the full hash instead of trimming the prefix, or encode the cost alongside the base64 so you don’t need heuristics.
🤖 Prompt for AI Agents
In zmsentities/src/Zmsentities/Apikey.php around lines 46 to 49, the
verification reconstructs a bcrypt prefix with a hardcoded cost of 12 which can
break verification for existing hashes created with different costs; update the
method to first validate that $hash is valid base64 and fail early if not, then
attempt password_verify using reconstructed prefixes for cost 12 and cost 10
(return true on the first match, false if both fail) to preserve compatibility;
additionally add a follow-up note to pin the cost in getHash() and/or store the
full bcrypt hash (or the cost) instead of relying on reconstructing the prefix.
| @@ -21,7 +21,7 @@ | |||
| "symfony/translation": "^5.2", | |||
| "symfony/twig-bridge": "^5.2", | |||
| "stevenmaguire/oauth2-keycloak": "^4.0", | |||
| "slim/psr7": "^1.5", | |||
| "slim/psr7": "^1.7.1", | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Slim PSR-7 bump looks good; verify latest stable and BC notes
Upgrading to ^1.7.1 is compatible with Slim 4. Suggest verifying the latest stable and whether a newer 1.x is available.
Run:
🏁 Script executed:
#!/bin/bash
# List PHP constraints across all composer.json
fd -a composer.json | while read -r f; do
php_req=$(jq -r '.require.php // empty' "$f")
if [ -n "$php_req" ]; then echo "$f -> $php_req"; fi
done
# Check if any composer.json still requires >=8.3
rg -nP '"php"\s*:\s*">=\s*8\.3"'
# Check latest slim/psr7 version on Packagist
curl -s https://repo.packagist.org/p/slim/psr7.json \
| jq -r '.packages["slim/psr7"] | to_entries | map(.key) | map(select(test("^\\d+\\.\\d+\\.\\d+$"))) | sort_by(split(".")|map(tonumber)) | last'Length of output: 3420
Invalid slim/psr7 version constraint
The Packagist registry shows the latest 1.x release of slim/psr7 is 1.7.0—not 1.7.1—so the current requirement will fail to resolve. Please update the constraint (or target 2.x if you’ve verified BC):
• File: zmsslim/composer.json
Line 24
- "slim/psr7": "^1.7.1",
+ "slim/psr7": "^1.7.0",Optionally, consider upgrading to "^2.0" after reviewing the slim/psr7 changelog for any breaking changes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "slim/psr7": "^1.7.1", | |
| "slim/psr7": "^1.7.0", |
🤖 Prompt for AI Agents
In zmsslim/composer.json around line 24, the slim/psr7 version constraint "
^1.7.1" is invalid because Packagist's latest 1.x release is 1.7.0; update the
requirement to a valid constraint such as "^1.7.0" (or change to "^2.0" only
after verifying compatibility), then run composer update to refresh
composer.lock and ensure the dependency installs correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
zmsdb/src/Zmsdb/Slot.php (3)
256-296: $status used before initialization in writeSlotListForDate$status is updated and returned but never initialized in this function, which can trigger notices and return null unexpectedly.
Initialize it up-front:
protected function writeSlotListForDate( \DateTimeInterface $time, Collection $slotlist, AvailabilityEntity $availability ) { $ancestors = []; $hasAddedSlots = false; + $status = false; $calendar = new OverallCalendar();
179-190: Ensure all callers supply the newslotLastChangeargumentRemoving the dead
if (!$slotLastChange)branch is correct given the signature change, but this makes the third parameter mandatory. You must update every call site to pass a valid\DateTimeInterface $slotLastChange.Please update:
- Tests in
zmsdb/tests/Zmsdb/SlotTest.php: everywriteByAvailability($availability, $now)call needs a third argument. For example:$lastChange = (new Slot())->readLastChangedTimeByAvailability($availability); $status = (new Slot())->writeByAvailability($availability, $now, $lastChange);- API hooks in
zmsapi/src/Zmsapi/AvailabilitySlotsUpdate.php: replacewith(new SlotRepository())->writeByAvailability($availability, \App::$now);$lastChange = (new SlotRepository())->readLastChangedTimeByAvailability($availability); (new SlotRepository())->writeByAvailability($availability, \App::$now, $lastChange);Once all callers are updated, drop the redundant null-check in
Slot::writeByAvailability.
378-394: CastScopeIDs to int at each call site
The new?int $scopeIDsignature will TypeError if$scope->idis a string (the default PDO fetch). Please update the two call sites inCalculateSlots.phpto explicitly cast:
- In
zmsdb/src/Zmsdb/Helper/CalculateSlots.phparound line 170:- if ($slotsProcessed = $slotQuery->updateSlotProcessMapping($scope->id)) {
- if ($slotsProcessed = $slotQuery->updateSlotProcessMapping((int) $scope->id)) {
Also consider adding `(int)` casts on `deleteSlotProcessOnSlot($scope->id)` (line 167) if that signature is ever tightened similarly. </blockquote></details> </blockquote></details>🧹 Nitpick comments (8)
zmsclient/src/Zmsclient/Psr7/RequestException.php (4)
12-17: Fix misleading PHPDoc (wrong param and outdated types)The docblock references a non-existent ResponseInterface parameter and has outdated/incorrect types/casing. Align it with the actual signature and nullable types.
Apply:
- /** - * @param String $message - * @param \Psr\Http\Message\ResponseInterface $response - * @param \Psr\Http\Message\RequestInterface $request (optional) reference for better error messages - * @param \Exception $previous - */ + /** + * @param string $message + * @param \Psr\Http\Message\RequestInterface|null $request Optional reference for better error messages + * @param \Throwable|null $previous + */
8-11: Type the $request property to match the constructor (nullable RequestInterface)The property is currently untyped and the docblock doesn’t reflect nullability. Add a property type for safety and update the @var annotation accordingly. This is non-breaking and improves static analysis.
Apply:
- /** - * @var \Psr\Http\Message\RequestInterface $request - */ - public $request; + /** + * @var \Psr\Http\Message\RequestInterface|null $request + */ + public ?\Psr\Http\Message\RequestInterface $request;Optional: If this property is never mutated post-construction, consider marking it as readonly in PHP 8.1+:
- public readonly ?\Psr\Http\Message\RequestInterface $request;
Only do this if no external code writes to this property after instantiation.
24-26: Remove unnecessary local $code variableTiny cleanup: no need for an intermediate $code since it’s always 0.
Apply:
- $code = 0; - parent::__construct($message, $code, $previous); + parent::__construct($message, 0, $previous);
19-21: Widen$previousto?\ThrowableSwitching the constructor’s
$previousfrom?\Exceptionto?\Throwablelets you chain both Errors and Exceptions, remains fully backward-compatible, and aligns with PHP best practices. I checked call sites inClient.php(line 37) and onlyExceptioninstances are passed, so this change is safe.Apply this diff:
--- a/zmsclient/src/Zmsclient/Psr7/RequestException.php +++ b/zmsclient/src/Zmsclient/Psr7/RequestException.php @@ public function __construct( - ?\Exception $previous = null + ?\Throwable $previous = nullzmsdb/src/Zmsdb/Slot.php (4)
205-209: Inconsistent use of OverallCalendar vs alias CalendarYou import OverallCalendar as Calendar (Line 10) but instantiate both Calendar and OverallCalendar directly. Prefer one for consistency.
Use the alias consistently:
- $calendar = new OverallCalendar(); + $calendar = new Calendar();and
- $calendar = new \BO\Zmsdb\OverallCalendar(); + $calendar = new Calendar();Also applies to: 264-266, 450-451
124-125: Typo in comment (“serval” → “several”) and minor grammarNit, but easy fix for readability.
- // Be aware, that last slot change and current time might differ serval days - // if the rebuild fails in some way + // Be aware that the last slot change and current time might differ several days + // if the rebuild fails in some way
85-85: Replace TODOs with tracked issuesTwo TODOs remain:
- Line 85: “remove after ZMS1 is deactivated”
- Line 293: “Check if slot changed before writing ancestor IDs”
Convert to tracked tickets with acceptance criteria and a plan, or address them now.
I can draft issue descriptions referencing affected code paths and propose acceptance criteria if you want.
Also applies to: 293-293
73-99: Explicit false return for hasScopeRelevantChanges improves clarityThe function currently returns true or null. Adding an explicit false clarifies intent and avoids tri-state semantics.
public function hasScopeRelevantChanges( \BO\Zmsentities\Scope $scope, \DateTimeInterface $slotLastChange ) { ... - } + } + return false; }📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
zmsadmin/src/Zmsadmin/ScopeAppointmentsByDayXlsExport.php(1 hunks)zmsclient/src/Zmsclient/Psr7/RequestException.php(1 hunks)zmsdb/src/Zmsdb/Slot.php(7 hunks)🚧 Files skipped from review as they are similar to previous changes (1)
- zmsadmin/src/Zmsadmin/ScopeAppointmentsByDayXlsExport.php
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit Configuration File
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsclient/src/Zmsclient/Psr7/RequestException.phpzmsdb/src/Zmsdb/Slot.php**/*.php
⚙️ CodeRabbit Configuration File
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...Files:
zmsclient/src/Zmsclient/Psr7/RequestException.phpzmsdb/src/Zmsdb/Slot.php⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: call-php-unit-tests / zmsapi-test
- GitHub Check: call-php-unit-tests / zmsdb-test
🔇 Additional comments (5)
zmsdb/src/Zmsdb/Slot.php (5)
249-251: Passing slotLastChange from writeByScope is correct and removes ambiguityThis makes the lifecycle explicit and avoids hidden reads of “last change” from within writeByAvailability. Good change.
103-104: AllisAvailabilityOutdatedcalls supply a non-null DateTimeInterfaceI’ve verified:
•
readLastChangedTimeByAvailability()always returns aDateTimeImmutable
• Every direct call toisAvailabilityOutdated(…, $now, $lastChange)—both in production code and all tests—passes aDateTimeInterfaceNo nulls can reach the
$slotLastChangeparameter. We can safely enforce it as non-null.
409-425: Verify callers fordeleteSlotProcessOnProcesssignature changeWe’ve tightened the signature to
?int $scopeID = null. Please confirm all call sites comply:
- zmsdb/src/Zmsdb/Helper/CalculateSlots.php:125
• Called with no arguments (defaults tonull— OK).- zmsdb/src/Zmsdb/Helper/CalculateSlots.php:163
• Passed$scope->id— ensureScope::$idis always anint.If
Scope::$idisn’t strictly typed asint, either cast it ((int)$scope->id) or revisit the signature.
75-76: hasScopeRelevantChanges call sites validatedI grepped the entire repo for hasScopeRelevantChanges and found only one caller in Slot.php around line 114, within isAvailabilityOutdated. That call passes $slotLastChange, which is itself a non-nullable DateTimeInterface in isAvailabilityOutdated’s signature. No other usages exist.
No dead null-path checks remain—type tightening is safe to merge.
396-407: deleteSlotProcessOnSlot callers verified – type tightening is safeAll invocations of
deleteSlotProcessOnSlotaccept onlynullor an integer identifier:
- In CalculateSlots.php:
deleteSlotProcessOnSlot()(uses defaultnull)deleteSlotProcessOnSlot($scope->id)($scope->idis the numeric scope ID from the database)No other callers pass non-int values. You can safely keep the
?intsignature.
…lotList - Fix writeByAvailability method calls in SlotTest to include required third parameter - Add explicit nullable types to SlotList constructor and getFreeProcesses method parameters - Resolve PHP 8.4 deprecation warnings about implicitly nullable parameters
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zmsdb/src/Zmsdb/Query/SlotList.php (1)
356-369: Nullability mismatch: getFreeProcesses allows null but dereferences itThe parameter is now nullable, but you immediately call ->format() on it. Guard null to avoid a fatal error.
Apply this diff:
public function getFreeProcesses( \BO\Zmsentities\Calendar $calendar, ?\DateTimeImmutable $freeProcessesDate = null, $slotType = 'public', $slotsRequired = 1 ) { - $selectedDate = $freeProcessesDate->format('Y-m-d'); + if (null === $freeProcessesDate) { + return []; + } + $selectedDate = $freeProcessesDate->format('Y-m-d'); $slotList = $this->slots[$selectedDate]; return $slotList->getFreeProcesses( $selectedDate, $this->scope, $this->availability, $slotType, $calendar['requests'], $slotsRequired ); }
🧹 Nitpick comments (4)
zmsdb/tests/Zmsdb/SlotTest.php (3)
279-279: OK to pass the same timestamp for now and lastChange here, but naming is confusingYou’re using $now (which is actually the last-changed timestamp) as both "now" and "slotLastChange". That’s fine for this specific test, but the variable name obscures intent. Consider renaming to clarify semantics.
289-289: Same as above: passing $now for both parameters is acceptable, but rename for clarity$now is actually the last-changed time in this context. Using a more descriptive variable name would make the test intention clearer.
557-558: Replace error_log with proper Monolog logging or remove debug outputTests should not use error_log(); prefer the project’s Monolog logger if available, or drop the debug output.
Apply this diff:
- error_log($debug); + if (class_exists('\App') && isset(\App::$log)) { + \App::$log->debug($debug); + }zmsdb/src/Zmsdb/Query/SlotList.php (1)
312-318: Align addToCalendar signature with new nullability (optional)To keep API consistent, consider typing $freeProcessesDate as ?\DateTimeImmutable in addToCalendar as well.
Example:
- public function addToCalendar( - \BO\Zmsentities\Calendar $calendar, - \DateTimeInterface $now, - $freeProcessesDate, + public function addToCalendar( + \BO\Zmsentities\Calendar $calendar, + \DateTimeInterface $now, + ?\DateTimeImmutable $freeProcessesDate, $slotType = 'public', $slotsRequired = 1 ) {If there are external callers depending on a looser type, keep as-is.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
zmsdb/src/Zmsdb/Query/SlotList.php(2 hunks)zmsdb/tests/Zmsdb/SlotTest.php(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit Configuration File
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsdb/src/Zmsdb/Query/SlotList.phpzmsdb/tests/Zmsdb/SlotTest.php
**/*.php
⚙️ CodeRabbit Configuration File
**/*.php: Flag any usage of error_log() as it should be replaced with proper Monolog logging mechanisms provided by zmsslim which should be setup in the App/Application of each module:
- For error handling: Use the proper Monolog logging framework with error levels
- For application info logs: Use the proper Monolog logging framework with info levels
- For debugging: Use a dedicated debug logger or remove debug statements
- For CLI output: Use a CLI output handler or symfony/console
- The application log levels are as follows DEBUG, INFO, NOTICE, WARNING, ERROR, CRITICAL, ALERT, and EMERGENCY
Example replacement:
// Instead of: error_log("Import failed - " . $e->getMessage()); // Use: $log->error("Import failed", ['error' => $e->getMessage()]);Flag specific logging violations:
- error_log(), var_dump(), print_r(), die(), exit() usage (except proper error logging in catch blocks)
- Any debug output that should use proper Monolog logging
- Debug constants like DEBUG = true
- Debug logging that should be removed in production
- Commented debug code that should be cleaned up
Example replacements:
// Instead of: error_log("Error occurred"); var_dump($data); die('debug point'); // Use: $log->error("Error occurred", ['context' => 'processing']); $log->debug('Data dump', ['data' => $data]); // Remove die() statements entirelyException handling should use proper logging:
// Instead of: try { $result = $this->process(); } catch (Exception $e) { error_log("Processing failed: " . $e->getMessage()); } // Use: try { $result = $this->process(); } catch (Exception $e) { $log->error("Processing failed", ['error' => $e->getMessage(), 'trace' => $e->getTraceAsString()]); } ```...
Files:
zmsdb/src/Zmsdb/Query/SlotList.phpzmsdb/tests/Zmsdb/SlotTest.php
🧬 Code Graph Analysis (1)
zmsdb/tests/Zmsdb/SlotTest.php (1)
zmsdb/src/Zmsdb/Slot.php (2)
writeByAvailability(176-238)readLastChangedTimeByAvailability(364-376)
🔇 Additional comments (3)
zmsdb/tests/Zmsdb/SlotTest.php (2)
466-466: Tests intentionally pass explicit slotLastChange for reproducibilityThe existing tests call writeByAvailability($availability, $now, $now) so that $slotLastChange is fully controlled (e.g. static::$now in time-based scenarios). The internal fallback to readLastChangedTimeByAvailability only applies when a falsy third parameter is provided—but in these tests we always pass a concrete DateTime object. Unconditionally replacing the third argument with a fresh DB call would:
- Be redundant in cases where $now was already read from the DB
- Break the “change-by-time” integration test that relies on passing a fixed timestamp (static::$now) instead of the actual DB value
No updates are required here.
Likely an incorrect or invalid review comment.
275-281: Verify correct slotLastChange across all writeByAvailability callsPlease update every call to writeByAvailability so that the third argument truly represents the last-change timestamp, not the current “now” time, to avoid masking slot-rebuild logic. In particular:
• zmsdb/tests/Zmsdb/SlotTest.php
– Lines 279, 289, 318, 326, 332, 466, 508: replace the second “$now” (or static::$now) with a dedicated $lastChange variable—e.g.:$lastChange = (new Slot())->readLastChangedTimeByAvailability($availability); $now = new \DateTimeImmutable(); // or whatever “current” time is needed $status = (new Slot())->writeByAvailability($availability, $now, $lastChange);• zmsapi/src/Zmsapi/AvailabilitySlotsUpdate.php
– Lines 73 and 84: the calls(new SlotRepository())->writeByAvailability($availability, \App::$now);now need a third $slotLastChange parameter (for example, pass the same \App::$now or look up the true last-change from your repository/service).
• Any other writeByAvailability usages (e.g., in src/Zmsdb/Slot.php’s loop) should already pass the proper $slotLastChange; please double-check.
After you’ve updated these, rerun:
rg -nP --type=php '\bwriteByAvailability\s*\('to confirm every site supplies the correct third argument.
zmsdb/src/Zmsdb/Query/SlotList.php (1)
179-186: Double‐check Query\SlotList constructor usages after making parameters nullableWe’ve updated Zmsdb\Query\SlotList::__construct so that
$start,$stop,$now,$availabilityand$scopeall default tonull. Callers that omit any of these may now get a different internal state or behavior (e.g. slot‐generation logic relying on non‐null dates or entities).I only found explicit instantiations in:
- zmsdb/tests/Zmsdb/SlotListTest.php
•new SlotList()
•new SlotList($slotData, $start, $stop, $now)- zmsdb/src/Zmsdb/Query/SlotList.php (self-calls/recursion)
No other
new \BO\Zmsdb\Query\SlotList(usages surfaced via ripgrep. Please:
- Audit any dynamic or container-based instantiations not caught by text search.
- Confirm callers that previously relied on implicit defaults still behave correctly when dates or entities are
null.- Add/update tests for null
$availabilityand$scopeto guard against subtle regressions.
| ?\DateTimeImmutable $stop = null, | ||
| ?\DateTimeInterface $now = null, | ||
| ?\BO\Zmsentities\Availability $availability = null, | ||
| ?\BO\Zmsentities\Scope $scope = null | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Nullable ctor params can cause TypeError in createSlots; derive defaults or enforce non-null
You made $start, $stop, and $now nullable, but they’re passed directly to createSlots(), which requires non-null DateTimeInterface. If any caller passes null, this will throw a TypeError.
Either keep these non-nullable or guard/derive sensible defaults before calling createSlots.
Apply this diff to derive defaults safely:
public function __construct(
array $slotData = ['availability__id' => null],
- ?\DateTimeImmutable $start = null,
- ?\DateTimeImmutable $stop = null,
- ?\DateTimeInterface $now = null,
- ?\BO\Zmsentities\Availability $availability = null,
- ?\BO\Zmsentities\Scope $scope = null
+ ?\DateTimeImmutable $start = null,
+ ?\DateTimeImmutable $stop = null,
+ ?\DateTimeInterface $now = null,
+ ?\BO\Zmsentities\Availability $availability = null,
+ ?\BO\Zmsentities\Scope $scope = null
) {
$this->availability = $availability;
$this->scope = $scope;
$this->setSlotData($slotData);
if ($this->availability && isset($this->availability['id'])) {
- $this->createSlots($start, $stop, $now);
+ if (null === $start || null === $stop || null === $now) {
+ $now = \BO\Zmsentities\Helper\DateTime::create($now ?? new \DateTimeImmutable());
+ $start = $start ?? $this->availability->getBookableStart($now)->modify('00:00:00');
+ $stop = $stop ?? $this->availability->getBookableEnd($now);
+ }
+ $this->createSlots($start, $stop, $now);
$this->addQueryData($slotData);
}
}🤖 Prompt for AI Agents
In zmsdb/src/Zmsdb/Query/SlotList.php around lines 182 to 186, the constructor
made $start, $stop, and $now nullable but they are passed into createSlots()
which requires non-null DateTimeInterface; change the constructor body to derive
sensible defaults when any of these are null (e.g., set $now = new
\DateTimeImmutable() if null, set $start = $now if null, set $stop =
$start->modify('+1 day') or another domain-appropriate default) or alternatively
validate and throw a clear exception if nulls are not acceptable; ensure after
these assignments you pass only non-null DateTimeInterface instances into
createSlots().
- Fix parameter passing in testChangeByTimeIntegration to use correct lastChange values - Ensure writeByAvailability method receives proper slotLastChange parameter for time comparison - Resolve test failure by using proper time difference detection logic
ThomasAFink
added
php
…er.lock for PHP 8.4
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli (2)
68-76: Bug: composer "require" handled as list; can crash when missing or malformedcomposer.json "require" is an object (map). Defaulting to [] and then assigning string keys will raise at runtime if a module lacks "require".
Apply this diff to make it safe and explicit:
- require = composer_content.get("require", []) - for dependency_key in require: + require = composer_content.get("require", {}) or {} + if not isinstance(require, dict): + raise TypeError(f'"require" in {module}/composer.json must be a JSON object') + for dependency_key in list(require.keys()): if dependency_key.startswith("eappointment/"): if module not in module_dependencies: module_dependencies[module] = [] module_dependencies[module].append(dependency_key) require[dependency_key] = "@dev" composer_content["require"] = require
160-163: Guard against IndexError when calling “cli modules loop npm” without subcommandIndexing commands[1] without a length check will crash if only “npm” is passed.
Apply this diff:
- if is_npm_command and commands[1] == "install": + if is_npm_command and len(commands) > 1 and commands[1] == "install": modified_commands = list(commands) + ["--legacy-peer-deps"] os.system(" ".join(modified_commands)) - elif is_npm_command and len(commands) > 1 and commands[1] == "build": + elif is_npm_command and len(commands) > 1 and commands[1] == "build":Optionally, handle the bare “npm” case with a short help or by exiting with a clear message.
🧹 Nitpick comments (1)
cli (1)
20-34: modules list verified with optional deduplication guardAll entries in the
moduleslist (cli, lines 20–34) are unique and every module directory and its composer.json file is present. To harden this against accidental duplicates in future edits while preserving order, you may wrap the literal in an order-preserving deduplicator:-modules = [ +modules = list(dict.fromkeys([ "mellon", "zmsslim", "zmsentities", "zmsdldb", "zmsdb", "zmsclient", "zmsadmin", "zmsapi", "zmscalldisplay", "zmsmessaging", "zmsstatistic", "zmscitizenapi", "zmsticketprinter", -] +]))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (12)
zmsadmin/composer.lockis excluded by!**/*.lockzmsapi/composer.lockis excluded by!**/*.lockzmscalldisplay/composer.lockis excluded by!**/*.lockzmscitizenapi/composer.lockis excluded by!**/*.lockzmsclient/composer.lockis excluded by!**/*.lockzmsdb/composer.lockis excluded by!**/*.lockzmsdldb/composer.lockis excluded by!**/*.lockzmsentities/composer.lockis excluded by!**/*.lockzmsmessaging/composer.lockis excluded by!**/*.lockzmsslim/composer.lockis excluded by!**/*.lockzmsstatistic/composer.lockis excluded by!**/*.lockzmsticketprinter/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
cli(1 hunks)zmsdb/tests/Zmsdb/SlotTest.php(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- zmsdb/tests/Zmsdb/SlotTest.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit Configuration File
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
cli
🔇 Additional comments (2)
cli (2)
22-26: Module order changes (zmsslim, zmsentities, zmsdldb, zmsdb, zmsclient moved up) — LGTMThe new order better reflects foundational libs being processed earlier, which is helpful for composer path setups and bulk ops.
32-32: Relocating zmscitizenapi after zmsstatistic — LGTMNo functional issues anticipated; loops will just process it in the new position.
…ries in Docker build - Add COMPOSER_MIRROR_PATH_REPOS=1 to Containerfile build stage - Ensures path repositories like eappointment/zmsslim are copied instead of symlinked - Fixes 'Class BO\Slim\Application not found' error in production - Maintains local development workflow with symlinks
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.resources/Containerfile (1)
8-8: Composer mirror-path repos env: good addition; optionally scope to the RUN or document intentSetting COMPOSER_MIRROR_PATH_REPOS=1 in the build stage is appropriate and helps avoid symlinks from path repos, improving reproducibility inside Docker layers. Since it’s only needed for the composer invocation in make live, consider either documenting the intent or scoping it to that RUN for tighter locality.
Option A — document intent (keeps current behavior):
+# Mirror Composer path repos (no symlinks) for reproducible Docker builds ENV COMPOSER_MIRROR_PATH_REPOS=1Option B — scope to the RUN (no persistent stage env):
Dockerfile snippet (outside the changed line):
RUN COMPOSER_MIRROR_PATH_REPOS=1 make live
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.resources/Containerfile(1 hunks)
- Fix missing --optimize-autoloader flag in live target of all module Makefiles - Ensures proper autoloader generation during production builds - Combined with COMPOSER_MIRROR_PATH_REPOS=1, this should resolve the BO\Slim\Application autoloading issue - Affects: zmsadmin, zmsstatistic, zmscitizenapi, zmsmessaging, zmsapi, zmscalldisplay
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
zmsapi/Makefile (1)
14-14: Minor hardening for non-interactive CI and faster runtime lookupsConsider adding --no-interaction and --no-ansi to keep CI logs clean and prevent prompts. If your codebase doesn’t rely on PSR-4 fallback discovery, you can also consider --classmap-authoritative for maximum runtime lookup speed, and --apcu-autoloader if APCu is available in production. These are optional, safe defaults in many deployments.
Apply this diff if acceptable:
- composer install --no-dev --prefer-dist --optimize-autoloader + composer install --no-dev --prefer-dist --optimize-autoloader --no-interaction --no-ansiIf you want the stronger autoloading guarantees (verify first in a staging env):
- composer install --no-dev --prefer-dist --optimize-autoloader + composer install --no-dev --prefer-dist --optimize-autoloader --classmap-authoritative --no-interaction --no-ansiAnd with APCu (only if APCu is enabled in the target environment):
- composer install --no-dev --prefer-dist --optimize-autoloader + composer install --no-dev --prefer-dist --optimize-autoloader --apcu-autoloader --no-interaction --no-ansizmscalldisplay/Makefile (2)
27-27: Consider --classmap-authoritative and non-interactive flags for faster, stricter prod installsFor live/production builds, these flags can further reduce autoload overhead and make the step more deterministic:
- --classmap-authoritative: avoid scanning the filesystem at runtime; only use the generated classmap (ensure no dynamic class loading is required).
- --no-interaction and --no-progress: cleaner, non-blocking logs in CI/CD.
Apply this diff if compatible with your runtime expectations:
- composer install --no-dev --prefer-dist --optimize-autoloader + composer install --no-dev --prefer-dist --optimize-autoloader --classmap-authoritative --no-interaction --no-progress
28-30: Optional: use npm ci for reproducible live installsFor live/CI environments, npm ci is faster and guarantees installs match package-lock.json. It avoids drift and surprises compared to npm install.
If your pipeline and lockfile are up to date, consider:
- npm install --legacy-peer-deps + npm ci --legacy-peer-depsNote: The follow-up requirejs installation without saving is fine; npm ci can still be used prior to that step.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
zmsadmin/Makefile(1 hunks)zmsapi/Makefile(1 hunks)zmscalldisplay/Makefile(1 hunks)zmscitizenapi/Makefile(1 hunks)zmsmessaging/Makefile(1 hunks)zmsstatistic/Makefile(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit Configuration File
**/*: Apply the following Clean Code guidelines to all files, as summarized by wojteklu's Clean Code gist:Code is clean if it can be understood easily – by everyone on the team. Clean code can be read and enhanced by a developer other than its original author. With understandability comes readability, changeability, extensibility and maintainability.
General rules
- Follow standard conventions.
- Keep it simple stupid. Simpler is always better. Reduce complexity as much as possible.
- Boy scout rule. Leave the campground cleaner than you found it.
- Always find root cause. Always look for the root cause of a problem.
Design rules
- Keep configurable data at high levels.
- Prefer polymorphism to if/else or switch/case.
- Separate multi-threading code.
- Prevent over-configurability.
- Use dependency injection.
- Follow Law of Demeter. A class should know only its direct dependencies.
Understandability tips
- Be consistent. If you do something a certain way, do all similar things in the same way.
- Use explanatory variables.
- Encapsulate boundary conditions. Boundary conditions are hard to keep track of. Put the processing for them in one place.
- Prefer dedicated value objects to primitive type.
- Avoid logical dependency. Don't write methods which works correctly depending on something else in the same class.
- Avoid negative conditionals.
Names rules
- Choose descriptive and unambiguous names.
- Make meaningful distinction.
- Use pronounceable names.
- Use searchable names.
- Replace magic numbers with named constants.
- Avoid encodings. Don't append prefixes or type information.
Functions rules
- Small.
- Do one thing.
- Use descriptive names.
- Prefer fewer arguments.
- Have no side effects.
- Don't use flag arguments. Split method into several independent methods that can b...
Files:
zmsapi/Makefilezmscitizenapi/Makefilezmsadmin/Makefilezmsmessaging/Makefilezmsstatistic/Makefilezmscalldisplay/Makefile
🔇 Additional comments (7)
zmsapi/Makefile (2)
14-14: Good call: optimizing the Composer autoloader for live buildsAdding --optimize-autoloader is a sensible production default and should reduce autoload overhead at runtime.
14-14: All Makefile live targets already use --optimize-autoloader
Verified that everycomposer installinvocation in each Makefile’s live target includes--optimize-autoloader. No further changes are needed.zmscitizenapi/Makefile (1)
14-14: LGTM: optimized autoloader for live buildsThe addition should improve runtime class loading performance in production.
zmsadmin/Makefile (1)
22-22: LGTM: enable optimized Composer autoloading in prodAppropriate for the live target and aligns with the repo-wide upgrade effort.
zmsstatistic/Makefile (1)
22-22: LGTM: production autoloader optimizationConsistent with other modules and beneficial for runtime performance.
zmsmessaging/Makefile (1)
13-13: LGTM: add --optimize-autoloader for liveSolid production default; aligns with other Makefile updates in this PR.
zmscalldisplay/Makefile (1)
27-27: Good production hardening: autoloader optimization addedAdding --optimize-autoloader to the live install improves classmap generation and runtime performance. Nice.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit