Skip to content
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.

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

EZP-22098 Fix_min_max_value_for_contenttype #773

Conversation

idriss69
Copy link
Contributor

JIRA link: https://jira.ez.no/browse/EZP-22098

Description

The goal here is to set 0 as the maximum or the minimum allowed value on a Float or Integer field definition.

@@ -2,7 +2,7 @@
/**
* File containing the Float converter
*
* @copyright Copyright (C) 1999-2014 eZ Systems AS. All rights reserved.
* @copyright Copyright (C) 1999-2013 eZ Systems AS. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Nope :-)

@idriss69
Copy link
Contributor Author

Now it is ready for a review and comments :) @andrerom @lolautruche @bdunogier

);
$floatValidatorParameters = array( 'minFloatValue' => false, 'maxFloatValue' => false );

if ( $storageDef->dataFloat4 & self::HAS_MIN_VALUE === self::HAS_MIN_VALUE )
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the === part work?

In legacy we usually have things like this:
if ( $clearCacheType & self::CLEAR_NODE_CACHE )

Copy link
Member

Choose a reason for hiding this comment

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

The fact that we did it one way in legacy is barely proof of anything ;-)

php > $flag = 2; $min = 1; $max = 2;
php > var_dump( $flag&$min );
int(0)
php > var_dump( $flag&$max );
int(2)

But the === could be removed, since we will either get 0 or the tested value.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, but if this is more readable I'm all for it, but I did not find it more readable :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I erased it :)

@andrerom
Copy link
Contributor

+1

@ezrobot
Copy link
Contributor

ezrobot commented Mar 13, 2014

This Pull Request does not respect our PHP Coding Standards, please, see the report below:

FILE: ...ublish/Core/Persistence/Legacy/Content/FieldValue/Converter/Integer.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 108 | ERROR | Whitespace found at end of line
--------------------------------------------------------------------------------

@idriss69
Copy link
Contributor Author

ready for review :)

@@ -119,8 +119,12 @@ public function validate( FieldDefinition $fieldDefinition, SPIValue $fieldValue

$validationErrors = array();

// 0 and False are unlimited value for maxIntegerValue
Copy link
Contributor

Choose a reason for hiding this comment

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

CS : indent this line :)

@bdunogier
Copy link
Member

Added little nitpicks, but no blocker for a +1 once they're fixed.

}

$fieldDef->fieldTypeConstraints->validators = array( self::FLOAT_VALIDATOR_IDENTIFIER => $intValidatorParameters );
Copy link
Member

Choose a reason for hiding this comment

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

Make sure you harmonize the way to assign validators, it's done a bit differently in both types.

@bdunogier
Copy link
Member

+1

1 similar comment
@dpobel
Copy link
Contributor

dpobel commented Mar 20, 2014

+1

dpobel added a commit that referenced this pull request Mar 20, 2014
…_of_contenttype

EZP-22098 Fix_min_max_value_for_contenttype
@dpobel dpobel merged commit c1527da into ezsystems:master Mar 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
9 participants