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

[BUG]: incorrect column type detection for mysql enums and sets #14691

Closed
DarkSilence opened this issue Jan 9, 2020 · 5 comments
Closed

[BUG]: incorrect column type detection for mysql enums and sets #14691

DarkSilence opened this issue Jan 9, 2020 · 5 comments
Assignees
Labels

Comments

@DarkSilence
Copy link

@DarkSilence DarkSilence commented Jan 9, 2020

Describe the bug

Phalcon interprets enum and set columns in MySql DB which contain int, double or float in its value lists as a numeric columns, because of the way Phalcon detects the type of a column in mysql. Which leads to incorrect non-null validation in _preSave method in model.zep when column is also a NOT NULL.

To Reproduce

CREATE TABLE `robots` (
    `id` INT NOT NULL AUTO_INCREMENT PRIMARY KEY ,
    `name` VARCHAR(50) NULL ,
    `power_type` ENUM('solar', 'fusion') NOT NULL,
    `power_source` ENUM('external', 'internal') NOT NULL,
) ENGINE = MyISAM;
class Robots extends Phalcon\Mvc\Model
{
    public $id;
    public $name;
    public $power_type;
    public $power_source;
}

$robot = new Robots;
$robot->name = 'r2d2';
$robot->power_type = 'fusion';
$robot->power_source = 'internal';

if(!$robot->save())
{
    echo join('<br/>', $robot->getMessages());
}
// power_source is required
// because power_source has internal option which contains int in its name.

Details

  • Phalcon version: (php --ri phalcon)

Phalcon is a full stack PHP framework, delivered as a PHP extension, offering lower resource consumption and high performance.
phalcon => enabled
Author => Phalcon Team and contributors
Version => 4.0.0
Build Date => Dec 21 2019 19:02:31
Powered by Zephir => Version 0.12.15-814db50

Directive => Local Value => Master Value
phalcon.db.escape_identifiers => On => On
phalcon.db.force_casting => Off => Off
phalcon.orm.case_insensitive_column_map => Off => Off
phalcon.orm.cast_last_insert_id_to_int => Off => Off
phalcon.orm.cast_on_hydrate => Off => Off
phalcon.orm.column_renaming => On => On
phalcon.orm.disable_assign_setters => Off => Off
phalcon.orm.enable_implicit_joins => On => On
phalcon.orm.enable_literals => On => On
phalcon.orm.events => On => On
phalcon.orm.exception_on_failed_save => Off => Off
phalcon.orm.exception_on_failed_metadata_save => On => On
phalcon.orm.ignore_unknown_columns => Off => Off
phalcon.orm.late_state_binding => Off => Off
phalcon.orm.not_null_validations => On => On
phalcon.orm.update_snapshot_on_save => On => On
phalcon.orm.virtual_foreign_keys => On => On
phalcon.warning.enable => On => On

  • PHP Version: (php -v)

PHP 7.2.11 (cli) (built: Oct 10 2018 02:04:04) ( NTS MSVC15 (Visual C++ 2017) x64 )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies
with Xdebug v2.7.0beta1, Copyright (c) 2002-2018, by Derick Rethans

  • Operating System: Windows 7
  • Installation type: Windows pre-compiled dll
  • Zephir version (if any): none
  • Server: Nginx
  • Other related info (Database, table schema): Mysql, MyISAM
@DarkSilence

This comment has been minimized.

Copy link
Author

@DarkSilence DarkSilence commented Jan 9, 2020

If someone have faced the same issue and you need an asap fix here's the solution. You need to override _preSave method in your affected models. Here's a PHP code I got from zep:


/**
 * Executes internal hooks before save a record
 */
protected function _preSave(\Phalcon\Mvc\Model\MetaDataInterface $metaData, bool $exists, $identityField): bool
{
	/**
	 * Run Validation Callbacks Before
	 */
	/**
	 * Call the beforeValidation
	 */
	if ($this->fireEventCancel("beforeValidation") === false) {
		return false;
	}

	/**
	 * Call the specific beforeValidation event for the current action
	 */
	if ($exists) {
		$eventName = "beforeValidationOnUpdate";
	} else {
		$eventName = "beforeValidationOnCreate";
	}

	if ($this->fireEventCancel($eventName) === false) {
		return false;
	}

	/**
	 * Check for Virtual foreign keys
	 */
	if ($this->_checkForeignKeysRestrict() === false) {
		
		return false;

	}

	/**
	 * Columns marked as not null are automatically validated by the ORM
	 */
	$notNull = $metaData->getNotNullAttributes($this);

	if (is_array($notNull)) {

		/**
		 * Gets the fields that are numeric, these are validated in a
		 * different way
		 */
		$dataTypeNumeric = $metaData->getDataTypesNumeric($this);

		$columnMap = $metaData->getColumnMap($this);

		/**
		 * Get fields that must be omitted from the SQL generation
		 */
		if ($exists) {
			$automaticAttributes = $metaData->getAutomaticUpdateAttributes($this);
		} else {
			$automaticAttributes = $metaData->getAutomaticCreateAttributes($this);
		}

		$defaultValues = $metaData->getDefaultValues($this);

		/**
		 * Get string attributes that allow empty strings as defaults
		 */
		$emptyStringValues = $metaData->getEmptyStringAttributes($this);

		$error = false;

		foreach ($notNull as $field) {

			$attributeField = $field;

			/**
			 * We don't check fields that must be omitted
			 */
			if (!isset($automaticAttributes[$attributeField])) {
				
				$isNull = false;

				/**
				 * Field is null when: 1) is not set, 2) is numeric but
				 * its value is not numeric, 3) is null or 4) is empty string
				 * Read the attribute from the this_ptr using the real or renamed name
				 */
				if (isset($this->{$attributeField})) {

					$value = $this->{$attributeField};

					/**
					 * Objects are never treated as null, numeric fields
					 * must be numeric to be accepted as not null
					 */
					if (!is_object($value)) {
						
						if (!isset($dataTypeNumeric[$field]) || $field === 'power_source') {
							if (isset($emptyStringValues[$field])) {
								if ($value === null) {
									$isNull = true;
								}
							} else {
								if ($value === null || ($value === "" && (!isset($defaultValues[$field]) || $value !== $defaultValues[$field]))) {
									$isNull = true;
								}
							}
						} else {
							if (!is_numeric($value)) {
								$isNull = true;
							}
						}
					}

				} else {

					$isNull = true;
				}

				if ($isNull) {
					if (!$exists) {
						/**
						 * The identity field can be null
						 */
						if ($field == $identityField) {
							continue;
						}

						/**
						 * The field have default value can be null
						 */
						if (isset($defaultValues[$field])) {
							continue;
						}
					}

					/**
					 * An implicit PresenceOf message is created
					 */
					$this->errorMessages[] = new \Phalcon\Messages\Message(
						$attributeField . " is required",
						$attributeField,
						"PresenceOf"
					);

					$error = true;
				}
			}
		}

		if ($error) {
			$this->fireEvent("onValidationFails");
			$this->_cancelOperation();

			return false;
		}
	}

	/**
	 * Call the main validation event
	 */
	if ($this->fireEventCancel("validation") === false) {

		$this->fireEvent("onValidationFails");
		return false;

	}

	/**
	 * Run Validation
	 */
	/**
	 * Run Validation Callbacks After
	 */
	if ($exists) {
		$eventName = "afterValidationOnUpdate";
	} else {
		$eventName = "afterValidationOnCreate";
	}

	if ($this->fireEventCancel($eventName) === false) {
		return false;
	}

	if ($this->fireEventCancel("afterValidation") === false) {
		return false;
	}

	/**
	 * Run Before Callbacks
	 */
	if ($this->fireEventCancel("beforeSave") === false) {
		return false;
	}

	$this->skipped = false;

	/**
	 * The operation can be skipped here
	 */
	if ($exists) {
		$eventName = "beforeUpdate";
	} else {
		$eventName = "beforeCreate";
	}

	if ($this->fireEventCancel($eventName) === false) {
		return false;
	}

	/**
	 * Always return true if the operation is skipped
	 */
	if ($this->skipped === true) {
		return true;
	}

	return true;
}

Just replace $field === 'power_source' with your column names.

@ruudboon

This comment has been minimized.

Copy link
Member

@ruudboon ruudboon commented Jan 9, 2020

It should be able to detect enum.

case memstr(columnType, "enum"):

Will have a look at it.

@DarkSilence

This comment has been minimized.

Copy link
Author

@DarkSilence DarkSilence commented Jan 9, 2020

It should be able to detect enum.

It detects enums, but if enum (or set) is defined with values which contain int (like "internal") Phalcon detects this enum as an INT. I think this is happening because checks for ints are placed above check for enums here

case memstr(columnType, "int"):

@ruudboon

This comment has been minimized.

Copy link
Member

@ruudboon ruudboon commented Jan 9, 2020

I meant it should detect it properly in any order. Our memstr is a bit too general.
I think we can solve this by looking only at the start of the string.

DESCRIBE robots
field type null key default extra
id int(11) NO PRI NULL auto_increment
name varchar(50) YES NULL
power_type enum('solar','fusion') NO NULL
power_source enum('external', 'internal') NO NULL
@ruudboon ruudboon self-assigned this Jan 9, 2020
@ruudboon ruudboon added the 4.0.1 label Jan 9, 2020
@ruudboon ruudboon added this to Working on it in Phalcon Roadmap Jan 9, 2020
@ruudboon ruudboon mentioned this issue Jan 9, 2020
4 of 5 tasks complete
ruudboon added a commit that referenced this issue Jan 9, 2020
Resolved #14691
@ruudboon

This comment has been minimized.

Copy link
Member

@ruudboon ruudboon commented Jan 9, 2020

Fixed in #14692. Will be part of the 4.0.1 release. Thnx for reporting @DarkSilence

@ruudboon ruudboon closed this Jan 9, 2020
Phalcon Roadmap automation moved this from Working on it to Implemented Jan 9, 2020
@niden niden moved this from Implemented to Released in Phalcon Roadmap Jan 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Phalcon Roadmap
  
Released
2 participants
You can’t perform that action at this time.