10000 [Validator] BIC remove unused sprintf and parameter by kaznovac · Pull Request #30667 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Validator] BIC remove unused sprintf and parameter #30667

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 26, 2019
Merged

[Validator] BIC remove unused sprintf and parameter #30667

merged 1 commit into from
Mar 26, 2019

Conversation

kaznovac
Copy link
Contributor
Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

removed surplus parameter and sprintf as there are no placeholders in message template

OskarStark
OskarStark approved these changes Mar 24, 2019
View reviewed changes
Copy link
Contributor
@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Good catch, can you please target 3.4 branch with this bugfix?

xabbuh
xabbuh reviewed Mar 24, 2019
View reviewed changes
src/Symfony/Component/Validator/Constraints/Bic.php Outdated
@@ -52,8 +52,8 @@ public function __construct($options = null)
@trigger_error(sprintf('Using the "%s" constraint without the "symfony/intl" component installed is deprecated since Symfony 4.2.', __CLASS__), E_USER_DEPRECATED);
}

if (isset($options['iban']) && isset($options['ibanPropertyPath'])) {
throw new ConstraintDefinitionException(sprintf('The "iban" and "ibanPropertyPath" options of the Iban constraint cannot be used at the same time.', self::class));
if (isset($options['iban'], $options['ibanPropertyPath'])) {
Copy link
Member

Choose a reason for hiding this comment

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

This change should be reverted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because I am interested, can you explain why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xabbuh can you please elaborate? I'm also interested :)

Copy link
Member

Choose a reason for hiding this comment

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

because it requires more knowledge about a syntax that is less common
the previous CS is obvious, this one is less obvious.

@kaznovac
Copy link
Contributor Author

@OskarStark the code in 3.4 branch does not have the code that contains tis bug, so there was no need to port this fix to 3.4 branch

https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/Validator/Constraints/Bic.php

<?php
/*
 * This file is part of the Symfony package.
 *
 * (c) Fabien Potencier <fabien@symfony.com>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */
namespace Symfony\Component\Validator\Constraints;
use Symfony\Component\Validator\Constraint;
/**
 * @Annotation
 * @Target({"PROPERTY", "METHOD", "ANNOTATION"})
 *
 * @author Michael Hirschler <michael.vhirsch@gmail.com>
 */
class Bic extends Constraint
{
    const INVALID_LENGTH_ERROR = '66dad313-af0b-4214-8566-6c799be9789c';
    const INVALID_CHARACTERS_ERROR = 'f424c529-7add-4417-8f2d-4b656e4833e2';
    const INVALID_BANK_CODE_ERROR = '00559357-6170-4f29-aebd-d19330aa19cf';
    const INVALID_COUNTRY_CODE_ERROR = '1ce76f8d-3c1f-451c-9e62-fe9c3ed486ae';
    const INVALID_CASE_ERROR = '11884038-3312-4ae5-9d04-699f782130c7';
    protected static $errorNames = [
        self::INVALID_LENGTH_ERROR => 'INVALID_LENGTH_ERROR',
        self::INVALID_CHARACTERS_ERROR => 'INVALID_CHARACTERS_ERROR',
        self::INVALID_BANK_CODE_ERROR => 'INVALID_BANK_CODE_ERROR',
        self::INVALID_COUNTRY_CODE_ERROR => 'INVALID_COUNTRY_CODE_ERROR',
        self::INVALID_CASE_ERROR => 'INVALID_CASE_ERROR',
    ];
    public $message = 'This is not a valid Business Identifier Code (BIC).';
}

@OskarStark
Copy link
Contributor

Thank you for your feedback, in this case it should be against 4.2 as a bugfix if I am right

I am currently on a phone 📱

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 25, 2019
@kaznovac kaznovac force-pushed the bic-constraint-fix branch from 0f929a3 to 131e495 Compare March 26, 2019 10:04
@kaznovac
Copy link
Contributor Author

@OskarStark
I believe master is the right branch as edited line is introduced in bb6be15 which targets Symfony 4.3

@fabpot
Copy link
Member
fabpot commented Mar 26, 2019

Thank you @kaznovac.

@fabpot fabpot merged commit 131e495 into symfony:master Mar 26, 2019
fabpot added a commit that referenced this pull request Mar 26, 2019
@fabpot
minor #30667 [Validator] BIC remove unused sprintf and parameter (kaz…
76260e7 
…novac)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Validator] BIC remove unused sprintf and parameter

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

removed surplus parameter and sprintf as there are no placeholders in message template

Commits
-------

131e495 [Validator] BIC remove unused sprintf and parameter
@kaznovac kaznovac deleted the bic-constraint-fix branch March 26, 2019 12:05
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@OskarStark OskarStark OskarStark approved these changes

@xabbuh xabbuh xabbuh left review comments

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
Bug Status: Reviewed Validator
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
6 participants
@kaznovac @OskarStark @fabpot @nicolas-grekas @xabbuh @carsonbot
0