Skip to content

Conversation

@sreichel
Copy link
Contributor

@sreichel sreichel commented Feb 7, 2025

Description (*)

Replace Zend_Validate with symfony/validator.

Added new helper Mage_Core_Helper_Validate that contains some wrapper methods for symfony validation.
Moved some parts of Zend_Validate_Abstract to Mage_Core_Helper_Validation_Abstract for some backwacrd compa. (may be removed later).

Related Pull Requests

@github-actions github-actions bot added Component: Core Relates to Mage_Core Component: Catalog Relates to Mage_Catalog Component: Checkout Relates to Mage_Checkout Component: Customer Relates to Mage_Customer Component: Eav Relates to Mage_Eav Component: Adminhtml Relates to Mage_Adminhtml Component: Api PageRelates to Mage_Api Component: Contacts Relates to Mage_Contacts Component: Admin Relates to Mage_Admin Component: Wishlist Relates to Mage_Wishlist Component: Review Relates to Mage_Review Component: Newsletter Relates to Mage_Newsletter Component: ImportExport Relates to Mage_ImportExport composer Relates to composer.json phpunit labels Feb 7, 2025
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 7, 2025

@sreichel sreichel marked this pull request as draft February 7, 2025 01:26
@Hanmac
Copy link
Contributor

Hanmac commented Feb 7, 2025

sad no symfony noises xD

Also, you might need to look at this:

@deprecated Calling `validate()` directly from rules is deprecated. Please use {@see \Respect\Validation\Validator::isValid()} instead.

@sreichel
Copy link
Contributor Author

sreichel commented Feb 7, 2025

sad no symfony noises xD

I tried, but writing rules is more intuitive with that ... imho.

Also, you might need to look at this:

@deprecated Calling `validate()` directly from rules is deprecated. Please use {@see \Respect\Validation\Validator::isValid()} instead.

I am aware of it, but latest version requies php 8.1.

@Hanmac
Copy link
Contributor

Hanmac commented Feb 7, 2025

for the current changes in this MR, Symfony Validator might be easy too.

the bigger use would be that we could validate entire objects like Customer Address at once

Also the Intl/Translation support is nice, so we could have the errors already translated by the System

@mattdavenport
Copy link
Contributor

I would be in favor of switching this to Symfony validation. Looking at this from a bigger picture, it would be best if we picked a library of components (e.g. Symfony/Laminas/etc.) and stuck to those throughout the project when replacing outdated libraries. IMO not doing this will increase the maintenance burden long term with differing packages/conventions to keep track of.

# Conflicts:
#	composer.json
#	composer.lock
@github-actions github-actions bot removed the PHPStorm label Oct 26, 2025
@sreichel sreichel changed the title Replace Zend_Validation Replace Zend_Validate with symfony/validator Oct 26, 2025
@sreichel sreichel marked this pull request as ready for review October 26, 2025 20:57
@sreichel sreichel requested review from Hanmac and Copilot October 26, 2025 20:57
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 replaces the legacy Zend_Validate validation framework with Symfony's symfony/validator component, modernizing the validation layer across the OpenMage codebase. The changes introduce a new helper class Mage_Core_Helper_Validate that wraps Symfony validation functionality, and updates all existing validation calls throughout the codebase to use this new approach.

Key changes include:

  • Added new Mage_Core_Helper_Validate helper with wrapper methods for common validation operations
  • Created Mage_Core_Helper_Validate_Abstract base class for backward compatibility with existing validator patterns
  • Migrated all Zend_Validate usage to Symfony validators across models, controllers, and helpers
  • Updated test data providers to reflect new validation error messages

Reviewed Changes

Copilot reviewed 71 out of 72 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
composer.json Added symfony/validator and symfony/mime dependencies
app/code/core/Mage/Core/Helper/Validate.php New validation helper with Symfony wrapper methods
app/code/core/Mage/Core/Helper/Validate/Abstract.php Abstract base class for custom validators
app/code/core/Mage/Core/Helper/Validate/Interface.php Interface defining validation contract
app/code/core/Mage/Core/Model/Abstract.php Added getValidationHelper() method for easy access
app/code/core/Mage/Customer/Model/Customer.php Migrated customer validation to new system
app/code/core/Mage/Newsletter/Model/Template.php Updated template validation logic
app/code/core/Mage/Oauth/Model/Token.php Replaced custom key length validators
tests/unit/Mage/Newsletter/Model/TemplateTest.php Updated test expectations for new error messages
Multiple test files Updated test data providers for new validation behavior
Comments suppressed due to low confidence (1)

app/code/core/Mage/Customer/controllers/AccountController.php:1

  • The removal of early return statements at lines 793 and 796 changes the control flow. While both code paths now fall through to implicit returns, this is less explicit than the original early returns and could make debugging more difficult. Consider keeping the explicit return statements for clarity.
<?php

Comment on lines +122 to 123
if (empty($data['attribute_set_id'])) {
$this->_critical('Missing "attribute_set_id" in request.', Mage_Api2_Model_Server::HTTP_BAD_REQUEST);
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

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

Changed from !isset($data['attribute_set_id']) || empty($data['attribute_set_id']) to just empty($data['attribute_set_id']). While functionally equivalent (empty() checks both isset and truthiness), this changes the behavior subtly: empty() will return true for '0' and 0, whereas the original would only check if the key exists and has a truthy value. Verify this doesn't break validation for legitimate zero values.

Suggested change
if (empty($data['attribute_set_id'])) {
$this->_critical('Missing "attribute_set_id" in request.', Mage_Api2_Model_Server::HTTP_BAD_REQUEST);
if (!isset($data['attribute_set_id']) || !is_numeric($data['attribute_set_id']) || (int)$data['attribute_set_id'] <= 0) {
$this->_critical('Missing or invalid "attribute_set_id" in request.', Mage_Api2_Model_Server::HTTP_BAD_REQUEST);

Copilot uses AI. Check for mistakes.
}

if (!isset($data['type_id']) || empty($data['type_id'])) {
if (empty($data['type_id'])) {
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

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

Changed from !isset($data['attribute_set_id']) || empty($data['attribute_set_id']) to just empty($data['attribute_set_id']). While functionally equivalent (empty() checks both isset and truthiness), this changes the behavior subtly: empty() will return true for '0' and 0, whereas the original would only check if the key exists and has a truthy value. Verify this doesn't break validation for legitimate zero values.

Copilot uses AI. Check for mistakes.
sreichel and others added 7 commits October 26, 2025 22:08
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
# Conflicts:
#	app/code/core/Mage/Review/Model/Review.php
#	app/code/core/Mage/Wishlist/controllers/IndexController.php
#	tests/unit/Mage/Newsletter/Model/TemplateTest.php
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
7.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

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

Labels

Component: Admin Relates to Mage_Admin Component: Adminhtml Relates to Mage_Adminhtml Component: Api PageRelates to Mage_Api Component: Api2 Relates to Mage_Api2 Component: Catalog Relates to Mage_Catalog Component: Checkout Relates to Mage_Checkout Component: Contacts Relates to Mage_Contacts Component: Core Relates to Mage_Core Component: Customer Relates to Mage_Customer Component: Dataflow Relates to Mage_Dataflow Component: Eav Relates to Mage_Eav Component: ImportExport Relates to Mage_ImportExport Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: Newsletter Relates to Mage_Newsletter Component: Oauth Relates to Mage_Oauth Component: Review Relates to Mage_Review Component: Widget Relates to Mage_Widget Component: Wishlist Relates to Mage_Wishlist composer Relates to composer.json environment new feature phpstan PHPStorm phpunit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants