migrate long rent command from legacy add request validation for sms senders in cli

[sveneld]

PR Type

Enhancement, Bug fix, Documentation

---

Description

• Migrated long rental check logic to a new Symfony command.

• Introduced event-driven architecture for long rental notifications.

• Updated SMS connectors to handle CLI scenarios gracefully.

• Enhanced configuration and documentation for user notifications.

---

Changes walkthrough 📝

	Relevant files
Enhancement	7 files common.php Removed legacy long rental check function.                              +0/-32    LongRentalCheckCommand.php Added new Symfony command for long rental checks.                +71/-8    LongRentEvent.php Introduced new event for long rental notifications.            +22/-0    AdminNotificationEventListener.php Added handler for long rental event notifications.              +19/-0    BikeRepository.php Added method to fetch rented bikes.                                            +16/-0    HistoryRepository.php Added method to fetch last bike rent by user.                        +21/-0    messages+intl-icu.en.php Updated translations for long rental notifications.            +5/-2
Configuration changes	5 files config.example.php Removed deprecated notify user configuration.                        +0/-2      services.php Registered new command and event listener services.            +5/-0      generate.php Updated configuration file inclusion logic.                            +1/-1      index.php Updated configuration file reference for installation.      +1/-1      .env.dist Added environment variable for user notification toggle.  +2/-0
Bug fix	4 files DebugConnector.php Improved SMS handling for CLI scenarios.                                  +5/-2      EuroSmsConnector.php Improved SMS handling for CLI scenarios.                                  +5/-2      LoopbackConnector.php Improved SMS handling for CLI scenarios.                                  +5/-2      TextmagicSmsConnector.php Improved SMS handling for CLI scenarios.                                  +5/-2
Documentation	1 files INSTALL.md Updated documentation for long rental notification configuration. +1/-1

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[qodo-merge-pro-for-open-source]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 Security concerns SQL injection: The HistoryRepository.php file contains a SQL query that directly concatenates user input (bikeNumber and userId) into the query string without proper escaping or parameter binding. This could allow SQL injection attacks if the input values are not properly validated upstream.

⚡ Recommended focus areas for review Error Handling The command does not handle database connection errors or SMS sending failures. Missing try-catch blocks around database queries and SMS operations could cause command failures. $abusers = []; $rentedBikes = $this->bikeRepository->findRentedBikes(); foreach ($rentedBikes as $row) { $bikeNumber = (int)$row['bikeNum']; $userId = (int)$row['userId']; $userName = $row['userName']; $userPhone = $row['number']; $lastRent = $this->historyRepository->findLastBikeRentByUser($bikeNumber, $userId); if (is_null($lastRent)) { $this->logger->error('Last rent not found for bike', compact('bikeNumber', 'userId')); continue; } $time = strtotime($lastRent['time']); if ($time + ($this->configuration->get('watches')['longrental'] * 3600) <= time()) { $abusers[] = [ 'userId' => $userId, 'bikeNumber' => $bikeNumber, 'userName' => $userName, 'userPhone' => $userPhone, ]; if ($this->notifyUser) { $this->smsSender->send( $userPhone, $this->translator->trans( 'Please, return your bike {bikeNumber} immediately to the closest stand! Ignoring this warning can get you banned from the system.', ['{bikeNumber}' => $bikeNumber] ) ); } } } Runtime Exception The connector throws a runtime exception when used from CLI, but this exception is not documented and may not be properly handled by calling code. if (is_null($this->request)) { throw new \RuntimeException('Could not receive sms in cli'); } SQL Injection The query in findLastBikeRentByUser() uses raw integer concatenation instead of prepared statements or parameter binding. WHERE bikeNum = $bikeNumber AND userId = $userId AND action = 'RENT'

[qodo-merge-pro-for-open-source]

CI Failure Feedback 🧐

(Checks updated until commit 07e404e)

Action: phpcs (7.4)

Failed stage: Run PHP CodeSniffer [❌]

Failed test name: php_codesniffer

Failure summary: The action failed due to a code style violation in the PHP CodeSniffer check: A line in file LongRentalCheckCommand.php (line 83) exceeds the maximum line length limit of 120 characters (actual length: 160 characters) While this is only a warning, the CI pipeline is configured to treat warnings as errors, causing the build to fail

Relevant error logs: 1: ##[group]Operating System 2: Ubuntu ... 184: - Locking symfony/asset (v5.4.45) 185: - Locking symfony/cache (v5.4.46) 186: - Locking symfony/cache-contracts (v2.5.4) 187: - Locking symfony/config (v5.4.46) 188: - Locking symfony/console (v5.4.47) 189: - Locking symfony/dependency-injection (v5.4.48) 190: - Locking symfony/deprecation-contracts (v2.5.4) 191: - Locking symfony/dotenv (v5.4.48) 192: - Locking symfony/error-handler (v5.4.46) ... 286: - Downloading symfony/polyfill-intl-grapheme (v1.31.0) 287: - Downloading symfony/string (v5.4.47) 288: - Downloading symfony/property-info (v5.4.48) 289: - Downloading symfony/property-access (v5.4.45) 290: - Downloading symfony/polyfill-php73 (v1.31.0) 291: - Downloading symfony/http-foundation (v5.4.48) 292: - Downloading symfony/event-dispatcher (v5.4.45) 293: - Downloading symfony/var-dumper (v5.4.48) 294: - Downloading symfony/error-handler (v5.4.46) ... 381: - Installing symfony/polyfill-intl-grapheme (v1.31.0): Extracting archive 382: - Installing symfony/string (v5.4.47): Extracting archive 383: - Installing symfony/property-info (v5.4.48): Extracting archive 384: - Installing symfony/property-access (v5.4.45): Extracting archive 385: - Installing symfony/polyfill-php73 (v1.31.0): Extracting archive 386: - Installing symfony/http-foundation (v5.4.48): Extracting archive 387: - Installing symfony/event-dispatcher (v5.4.45): Extracting archive 388: - Installing symfony/var-dumper (v5.4.48): Extracting archive 389: - Installing symfony/error-handler (v5.4.46): Extracting archive ... 446: shell: /usr/bin/bash -e {0} 447: env: 448: COMPOSER_PROCESS_TIMEOUT: 0 449: COMPOSER_NO_INTERACTION: 1 450: COMPOSER_NO_AUDIT: 1 451: ##[endgroup] 452: FILE: ...SourceBikeShare/OpenSourceBikeShare/src/Command/LongRentalCheckCommand.php 453: -------------------------------------------------------------------------------- 454: FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE 455: -------------------------------------------------------------------------------- 456: 83 | WARNING | Line exceeds 120 characters; contains 160 characters 457: -------------------------------------------------------------------------------- 458: Time: 2.77 secs; Memory: 14MB 459: ##[error]Process completed with exit code 1.

---

✨ CI feedback usage guide:

---

The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
The tool analyzes the failed checks and provides several feedbacks:

• Failed stage
• Failed test name
• Failure summary
• Relevant error logs

In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

/checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"

where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

Configuration options

• enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
• excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
• enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
• persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
• final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

See more information about the checks tool in the docs.

[qodo-merge-pro-for-open-source]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Add error handling for SMS sending failures to prevent command execution interruption Add error handling for the case when smsSender->send() fails to notify a user about long rental. This could prevent the command from failing if SMS sending fails for one user. src/Command/LongRentalCheckCommand.php [79-88] if ($this->notifyUser) { - $this->smsSender->send( - $userPhone, - $this->translator->trans( - 'Please, return your bike {bikeNumber} immediately to the closest stand! Ignoring this warning can get you banned from the system.', - ['{bikeNumber}' => $bikeNumber] - ) - ); + try { + $this->smsSender->send( + $userPhone, + $this->translator->trans( + 'Please, return your bike {bikeNumber} immediately to the closest stand! Ignoring this warning can get you banned from the system.', + ['{bikeNumber}' => $bikeNumber] + ) + ); + } catch (\Exception $e) { + $this->logger->error('Failed to send SMS notification about long rental', [ + 'error' => $e->getMessage(), + 'userPhone' => $userPhone, + 'bikeNumber' => $bikeNumber + ]); + } } Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion adds critical error handling to prevent the command from failing when SMS sending fails, which could affect the processing of other users. The logging of failures also helps with debugging.	8
General	Validate input data to prevent processing of invalid bike numbers or user IDs Add input validation for bike number and user ID to ensure they are positive integers before processing. src/Command/LongRentalCheckCommand.php [60-61] $bikeNumber = (int)$row['bikeNum']; $userId = (int)$row['userId']; +if ($bikeNumber <= 0 || $userId <= 0) { + $this->logger->error('Invalid bike number or user ID', [ + 'bikeNumber' => $bikeNumber, + 'userId' => $userId + ]); + continue; +} Apply this suggestion Suggestion importance[1-10]: 7 Why: Input validation is important for data integrity and prevents processing of invalid data that could cause issues downstream. The suggestion includes appropriate error logging.	7