Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 26, 2025

Summary of Changes

Improved date handling robustness throughout OpenMage by adding comprehensive InvalidFormatException handling for all Carbon::parse() calls across core modules.

Note: The original installation crash fix in app/Mage.php has been removed from this PR and will be addressed separately.

Files Modified (19 files)

Installation ✅

  • app/code/core/Mage/Install/Model/Installer/Config.php - Protected replaceTmpInstallDate() (Rector fix: added blank line)

Date/Time Core Functions ✅

  • app/code/core/Mage/Core/Model/Date.php - Protected gmtTimestamp() and timestamp() (PHPStan fix: updated return type)
  • app/code/core/Mage/Core/Model/Locale.php - Protected storeTimeStamp() and isStoreDateInInterval() (Fixed: initialized variables to prevent undefined warnings)
  • lib/Varien/Db/Adapter/Mysqli.php - Protected convertDate() and convertDateTime()
  • app/code/core/Mage/Core/Block/Html/Date.php - Protected getEscapedValue()

Business Logic ✅

  • app/code/core/Mage/Eav/Model/Attribute/Data/Date.php - Protected date validation (PHP CS Fixer fix: corrected indentation)
  • app/code/core/Mage/CatalogRule/Model/Resource/Rule.php - Protected rule date processing (Rector fix: added blank line)
  • app/code/core/Mage/CatalogRule/Model/Rule/Condition/Product.php - Protected _prepareDatetimeValue()
  • app/code/core/Mage/Rule/Model/Condition/Product/Abstract.php - Protected validate()
  • app/code/core/Mage/Api/Model/Session.php - Protected isSessionExpired()

External Data Sources ✅

  • app/code/core/Mage/Shipping/Block/Tracking/Popup.php - Protected delivery date formatting
  • app/code/core/Mage/Usa/Model/Shipping/Carrier/Fedex.php - Protected tracking dates (Rector fix: added blank lines)
  • app/code/core/Mage/Usa/Model/Shipping/Carrier/Dhl/Abstract.php - Protected _determineShippingDay()
  • app/code/core/Mage/Usa/Model/Shipping/Carrier/Dhl/International.php - Protected _getQuotes() (Rector fix: added blank line)
  • app/code/core/Mage/AdminNotification/Model/Feed.php - Protected RSS date parsing

Payment & Background Jobs ✅

  • app/code/core/Mage/Paypal/Model/Cert.php - Protected getCertPath() (Rector fix: added blank line)
  • app/code/core/Mage/Paypal/Model/Express.php - Protected canCapture()
  • app/code/core/Mage/Cron/Model/Observer.php - Protected cleanup date handling (Fixed: improved logic to check empty dates before parsing)
  • app/code/core/Mage/Cron/Model/Schedule.php - Protected trySchedule()

Quality Checks ✅

  • All 19 files pass PHP syntax validation
  • Code review issues addressed:
    • Fixed potential undefined variable issue in Locale.php by initializing variables before try-catch
    • Improved Cron Observer logic to properly handle empty and malformed dates
  • Security check (CodeQL) - no issues detected
  • PHPStan error fixed - timestamp() return type updated to false|int
  • PHP CS Fixer error fixed - corrected indentation in EAV Date validation
  • Rector errors fixed - added blank lines after catch blocks (NewlineAfterStatementRector)
  • Consistent exception handling pattern across all files
  • Graceful fallbacks with appropriate defaults

Impact

✅ Prevents crashes from malformed dates in external API responses (shipping carriers, RSS feeds, etc.)
✅ More robust date handling throughout the application
✅ Improved cron history cleanup logic to properly handle malformed date records
✅ No breaking changes - all changes are backwards compatible

Scope Note

This PR focuses on adding comprehensive exception handling for Carbon::parse() calls throughout the codebase. The original installation crash issue mentioned in the linked issue will be addressed in a separate PR with appropriate tests.

Original prompt

This section details on the original issue you should resolve

<issue_title>[BUG] Installation fails due to unhandled Carbon parse exception in Mage::isInstalled()</issue_title>
<issue_description>### Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

During a fresh OpenMage installation, the application crashes with a fatal error caused by an unhandled Carbon::parse() exception in Mage::isInstalled().

When the installer writes temporary placeholder values (e.g. d-d-d-d-d) into etc/local.xml for the <install><date> node, Carbon::parse() fails and throws an exception. This exception is not caught and stops the bootstrap process, preventing the installation from completing.

Expected Behavior

  • OpenMage should not crash during installation
  • Temporary or invalid install date values should be treated as “not installed yet”
  • The installation process should continue normally
  • Behavior should be consistent with Magento 1, where Zend_Date handled such values gracefully

Steps To Reproduce

  1. Clone the OpenMage repository
  2. Set up a web server and open /index.php in a browser
  3. Start the installation wizard
  4. During installation, OpenMage creates etc/local.xml with a temporary placeholder value for <install><date> (e.g. d-d-d-d-d)
  5. Installation fails with a Carbon parsing exception and a fatal error

Environment

- OpenMage: latest downloaded today
- Php: 8.4.14
- Fresh install (no existing local.xml)

Anything else?

This issue was discovered during a fresh clone and first-time installation attempt. The problem was introduced by the migration from Zend_Date to Carbon in this PR #5131. Unlike Zend_Date, Carbon throws exceptions for invalid date strings, and the current implementation of Mage::isInstalled() does not guard against this behavior. As a result, the bootstrap process becomes fragile whenever etc/local.xml contains temporary, incomplete, or invalid values (e.g. during installation, interrupted installs, or filesystem latency).

Proposed Solution
Wrap the Carbon::parse() call in Mage::isInstalled() with basic validation and a try/catch block. Invalid or placeholder date values should be treated as not installed, instead of causing a fatal error. This restores the safe behavior previously provided by Zend_Date and prevents installation and bootstrap failures without affecting valid installations.

public static function isInstalled($options = [])
{
    if (self::$_isInstalled === null) {
        self::setRoot();

        if (is_string($options)) {
            $options = ['etc_dir' => $options];
        }

        $etcDir = self::getRoot() . DS . 'etc';
        if (!empty($options['etc_dir'])) {
            $etcDir = $options['etc_dir'];
        }

        $localConfigFile = $etcDir . DS . 'local.xml';

        self::$_isInstalled = false;

        if (is_readable($localConfigFile)) {
            $localConfig = simplexml_load_file($localConfigFile);

            if ($localConfig && isset($localConfig->global->install->date)) {
                $date = (string) $localConfig->global->install->date;

                // Installer writes temporary placeholders (e.g. d-d-d-d-d)
                // which must NOT be treated as a valid installation date
                if ($date !== '') {
                    try {
                        date_default_timezone_set('UTC');

                        $timestamp = Carbon::parse($date)->getTimestamp();
                        if ($timestamp > 0) {
                            self::$_isInstalled = true;
                        }
                    } catch (\Exception $e) {
                        // Invalid install date -> application is NOT installed
                        self::$_isInstalled = false;
                    }
                }
            }
        }
    }

    return self::$_isInstalled;
}
```</issue_description>

<agent_instructions>Check the whole OpenMage source code for other places affected by Carbon migration.</agent_instructions>

## Comments on the Issue (you are @copilot in this section)

<comments>
</comments>

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@github-actions github-actions bot added Component: PayPal Relates to Mage_Paypal Component: Core Relates to Mage_Core Component: lib/Varien Relates to lib/Varien Component: Usa Relates to Mage_Usa Component: Eav Relates to Mage_Eav Mage.php Relates to app/Mage.php Component: Api PageRelates to Mage_Api Component: CatalogRule Relates to Mage_CatalogRule Component: Shipping Relates to Mage_Shipping Component: Rule Relates to Mage_Rule Component: Install Relates to Mage_Install Component: lib/* Relates to lib/* labels Dec 26, 2025
@github-actions github-actions bot added Component: AdminNotification Relates to Mage_AdminNotification Component: Cron Relates to Mage_Cron labels Dec 26, 2025
Copilot AI changed the title [WIP] Fix installation failure due to unhandled Carbon parse exception Fix unhandled Carbon::parse() exceptions causing installation crashes Dec 26, 2025
Copilot AI requested a review from addison74 December 26, 2025 22:43
@OpenMage OpenMage deleted a comment from Copilot AI Dec 26, 2025
@OpenMage OpenMage deleted a comment from Copilot AI Dec 26, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Dec 26, 2025

Test Results

966 tests  ±0   958 ✅ ±0   16s ⏱️ ±0s
213 suites ±0     8 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 0f4897f. ± Comparison against base commit b74526a.

♻️ This comment has been updated with latest results.

@addison74 addison74 marked this pull request as ready for review December 26, 2025 23:36
Copilot AI review requested due to automatic review settings December 26, 2025 23:36
@addison74 addison74 requested a review from sreichel December 26, 2025 23:36
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a critical installation crash caused by unhandled Carbon::parse() exceptions when processing invalid or placeholder date values. The fix adds comprehensive InvalidFormatException handling to all Carbon::parse() calls across 20 files, ensuring graceful fallback behavior instead of fatal errors.

Key changes:

  • Added exception handling for installation date validation in app/Mage.php to prevent crashes during setup
  • Protected all date parsing operations throughout core modules (date/time utilities, business logic, external API integrations, payment processing, and background jobs)
  • Implemented consistent fallback values (null, false, current timestamp, or empty arrays) appropriate to each use case

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
app/Mage.php Wrapped installation date check with try-catch to handle invalid placeholder dates during installation
app/code/core/Mage/Install/Model/Installer/Config.php Protected replaceTmpInstallDate() against invalid date parsing
app/code/core/Mage/Core/Model/Date.php Added exception handling to gmtTimestamp() and timestamp() methods with false return on failure
app/code/core/Mage/Core/Model/Locale.php Protected storeTimeStamp() and isStoreDateInInterval() from invalid date formats
app/code/core/Mage/Core/Block/Html/Date.php Wrapped getEscapedValue() to fallback to escaped raw value on parse failure
app/code/core/Mage/Eav/Model/Attribute/Data/Date.php Added exception handling for date range validation with user-friendly error message
app/code/core/Mage/CatalogRule/Model/Resource/Rule.php Protected rule date processing, silently skipping rules with invalid dates
app/code/core/Mage/CatalogRule/Model/Rule/Condition/Product.php Wrapped _prepareDatetimeValue() to return null on invalid dates
app/code/core/Mage/Rule/Model/Condition/Product/Abstract.php Protected validate() method from invalid datetime attributes
app/code/core/Mage/Api/Model/Session.php Handled invalid dates in isSessionExpired() by treating as expired
app/code/core/Mage/Shipping/Block/Tracking/Popup.php Protected delivery date/time formatting from invalid external data
app/code/core/Mage/Usa/Model/Shipping/Carrier/Fedex.php Added exception handling for FedEx tracking response dates
app/code/core/Mage/Usa/Model/Shipping/Carrier/Dhl/Abstract.php Protected shipping day determination from invalid dates
app/code/core/Mage/Usa/Model/Shipping/Carrier/Dhl/International.php Wrapped quote date calculation with loop break on failure
app/code/core/Mage/AdminNotification/Model/Feed.php Protected RSS feed date parsing with fallback to current timestamp
app/code/core/Mage/Paypal/Model/Cert.php Wrapped certificate timestamp parsing with fallback to current time
app/code/core/Mage/Paypal/Model/Express.php Protected order authorization expiry check from invalid dates
app/code/core/Mage/Cron/Model/Observer.php Added exception handling to cleanup date processing
app/code/core/Mage/Cron/Model/Schedule.php Protected trySchedule() from invalid date strings
lib/Varien/Db/Adapter/Mysqli.php Wrapped convertDate() and convertDateTime() to return null on invalid input

Comment on lines +894 to +899
try {
$fromTimeStamp = Carbon::parse((string) $dateFrom)->getTimestamp();
$toTimeStamp = Carbon::parse((string) $dateTo)->getTimestamp();
} catch (InvalidFormatException) {
return false;
}
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The try-catch block catches the InvalidFormatException and returns false, but the variables $fromTimeStamp and $toTimeStamp will not be defined if the exception is thrown. This means that line 907 will reference undefined variables when is_empty_date returns false. The code should either initialize these variables before the try-catch or check their existence before using them on lines 907-908.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@sreichel
Copy link
Contributor

This PR is more about to adding try/catch to Carbon::parse, not only fixing the install bug :)

Thats defenitly a good idea, but please wait for next date related PR. I also try to write some more tests.

@addison74
Copy link
Contributor

@copilot - revert all changes you made to the Mage.php file and keep the others. Exclude the file from this PR.

Copy link
Contributor Author

Copilot AI commented Dec 28, 2025

@copilot - revert all changes you made to the Mage.php file and keep the others. Exclude the file from this PR.

Reverted in commit 68fd6ac. All changes to app/Mage.php have been removed from this PR. The file is now excluded and the PR focuses only on the other 19 files with Carbon::parse() exception handling.

@github-actions github-actions bot removed the Mage.php Relates to app/Mage.php label Dec 28, 2025
Copilot AI changed the title Fix unhandled Carbon::parse() exceptions causing installation crashes Add comprehensive Carbon::parse() exception handling across core modules Dec 28, 2025
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: AdminNotification Relates to Mage_AdminNotification Component: Api PageRelates to Mage_Api Component: CatalogRule Relates to Mage_CatalogRule Component: Core Relates to Mage_Core Component: Cron Relates to Mage_Cron Component: Eav Relates to Mage_Eav Component: Install Relates to Mage_Install Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: PayPal Relates to Mage_Paypal Component: Rule Relates to Mage_Rule Component: Shipping Relates to Mage_Shipping Component: Usa Relates to Mage_Usa

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Installation fails due to unhandled Carbon parse exception in Mage::isInstalled()

3 participants