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

Entity->hasValue() should not set null value for uninitialized property #514

Merged
merged 3 commits into from
Apr 15, 2021

Conversation

mabar
Copy link
Contributor

@mabar mabar commented Apr 5, 2021

Entity->hasValue() should not set value for property which does not have value yet. In most cases it will just set null and gets eventually overriden, but in case of enum wrapper which expects fixed subset of values, null (which is out of that subset) raises an exception when enum is tried to be created.

To verify issue, drop the source change in ImmutableDataTrait - included test will fail.

@mabar
Copy link
Contributor Author

mabar commented Apr 5, 2021

To understand my intention - I have modified entities to not set value (and generate useless sql query) in case the set value is same with current value. Thanks to that for all properties is called hasValue() before the property is set. Currently it fails with enums.

use Nextras\Orm\Entity\Entity;

abstract class BaseEntity extends Entity
{

	/**
	 * @return $this
	 */
	public function setModifiedValue(string $name, mixed $value): self
	{
		if (!$this->hasValue($name) || $this->getValue($name) !== $value) {
			$this->setValue($name, $value);
		}

		return $this;
	}

	public function __set(string $name, mixed $value): void
	{
		$this->setModifiedValue($name, $value);
	}

}

@hrach hrach force-pushed the bug/has-value-prop-init branch 2 times, most recently from a7f60ef to c44ea71 Compare April 15, 2021 21:02
@hrach hrach force-pushed the bug/has-value-prop-init branch from c44ea71 to f8a3ca4 Compare April 15, 2021 21:04
@hrach hrach force-pushed the bug/has-value-prop-init branch from f8a3ca4 to 1200d20 Compare April 15, 2021 21:08
@hrach
Copy link
Member

hrach commented Apr 15, 2021

Little more tough than I've expected but should be fixed now.

@hrach hrach enabled auto-merge April 15, 2021 21:08
@hrach
Copy link
Member

hrach commented Apr 15, 2021

Probably I leave it only in master since it requires more changes and especially in area that got changed in master.

@hrach hrach disabled auto-merge April 15, 2021 21:13
@hrach hrach merged commit bcf9743 into nextras:master Apr 15, 2021
@mabar
Copy link
Contributor Author

mabar commented Apr 16, 2021

It looks great, thank you

@mabar mabar deleted the bug/has-value-prop-init branch April 16, 2021 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants