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

Bind Param types ignored for non-scalar property values #13058

Closed
lcdennison opened this issue Sep 4, 2017 · 8 comments
Closed

Bind Param types ignored for non-scalar property values #13058

lcdennison opened this issue Sep 4, 2017 · 8 comments
Assignees
Labels
bug A bug report status: low Low
Milestone

Comments

@lcdennison
Copy link

lcdennison commented Sep 4, 2017

Expected and Actual Behavior

This is a related/continuation of #2752. Consider these two cases below.

Here we have a custom DateTime_Custom class that simply adds a __toString() method for conversion (since the base DateTime class doesn't have that).

class DateTime_Custom extends \DateTime {
	public function __toString(): string {
		return $this->format('Y-m-d H:m:s');
	}
}

Here we have case 1, where we set a property (i.e. DB field) to the DateTime_Custom object.

$robot = Robot()::findFirst();
$robot->datetime_field = new DateTime_Custom('now');
$robot->save();

This produces a SQL error because the field value for datetime_field is not bound or escaped. This is the specific SQL it produces:

UPDATE `robots` SET `datetime_field` = 2017-09-04 00:56:31 WHERE `id` = ?

However, if we force a cast during the assignment of the property, like this:

$robot = Robot()::findFirst();
$robot->datetime_field = (string) new DateTime_Custom('now');
$robot->save();

Then, the value is properly bound and the SQL produced is:

UPDATE `robots` SET `datetime_field` = ? WHERE `id` = ?

But, why do we need to force a cast to string during assignment of the property? The whole point is for us to be able to interact with the property as a DateTime_Custom object, and then Phalcon converts it to a string (by the DateTime_Custom->__toString() method) when it actually prepares SQL for saving. By the first case above, Phalcon is clearly aware of the __toString() method because it's giving the proper date value. The problem is that Phalcon is not properly understanding that it's a string and binding it as such.

The bind param type within the modelsMetadata->getBindTypes() method is listed as BIND_PARAM_STR for the datetime_field property. So, why doesn't Phalcon know to bind as a string on model save, same as always? That seems like a bug or piece of missing functionality to me. If I'm going wrong somewhere with this logic, please let me know your thoughts.

Details

  • Phalcon version: 3.2.2
  • PHP Version: 7.1.9
  • Operating System: CentOS 7.3.1611
  • Installation type: Installed via REMI php71-phalcon package
  • Server: Apache 2.4 with php-fpm
@stale
Copy link

stale bot commented Apr 16, 2018

Thank you for contributing to this issue. As it has been 90 days since the last activity, we are automatically closing the issue. This is often because the request was already solved in some way and it just wasn't updated or it's no longer applicable. If that's not the case, please feel free to either reopen this issue or open a new one. We will be more than happy to look at it again! You can read more here: https://blog.phalconphp.com/post/github-closing-old-issues

@stale stale bot added the stale Stale issue - automatically closed label Apr 16, 2018
@lcdennison
Copy link
Author

This should not be closed as it is still an open issue. Is there any update on a resolution to this?

@stale stale bot removed the stale Stale issue - automatically closed label Apr 16, 2018
@stale
Copy link

stale bot commented Jul 15, 2018

Thank you for contributing to this issue. As it has been 90 days since the last activity, we are automatically closing the issue. This is often because the request was already solved in some way and it just wasn't updated or it's no longer applicable. If that's not the case, please feel free to either reopen this issue or open a new one. We will be more than happy to look at it again! You can read more here: https://blog.phalconphp.com/post/github-closing-old-issues

@stale stale bot added the stale Stale issue - automatically closed label Jul 15, 2018
@stale stale bot closed this as completed Jul 16, 2018
@sergeyklay sergeyklay reopened this Jul 16, 2018
@stale stale bot removed the stale Stale issue - automatically closed label Jul 16, 2018
@stale
Copy link

stale bot commented Oct 14, 2018

Thank you for contributing to this issue. As it has been 90 days since the last activity, we are automatically closing the issue. This is often because the request was already solved in some way and it just wasn't updated or it's no longer applicable. If that's not the case, please feel free to either reopen this issue or open a new one. We will be more than happy to look at it again! You can read more here: https://blog.phalconphp.com/post/github-closing-old-issues

@stale stale bot added the stale Stale issue - automatically closed label Oct 14, 2018
@stale stale bot closed this as completed Oct 15, 2018
@lcdennison
Copy link
Author

Once again, this should not be closed. It is still an active issue to my knowledge.

@niden niden reopened this Oct 18, 2018
@stale stale bot removed the stale Stale issue - automatically closed label Oct 18, 2018
@niden
Copy link
Member

niden commented Oct 18, 2018

Thanks @ldennison I will try and get to it as soon as I can.

@niden niden self-assigned this Oct 18, 2018
CameronHall added a commit to CameronHall/cphalcon that referenced this issue Oct 31, 2018
CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 7, 2018
CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 7, 2018
@sergeyklay sergeyklay added this to the 3.4.2 milestone Nov 7, 2018
CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 7, 2018
Added note about fixing phalcon#13058
CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 7, 2018
Wrote unit tests for issue phalcon#13058
Update CHANGELOG-3.4.md

Added note about fixing phalcon#13058
CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 7, 2018
Wrote unit tests for issue phalcon#13058
Update CHANGELOG-3.4.md
niden pushed a commit that referenced this issue Nov 7, 2018
Wrote unit tests for issue #13058
Update CHANGELOG-3.4.md

Added note about fixing #13058
CameronHall added a commit to CameronHall/cphalcon that referenced this issue Nov 12, 2018
Wrote unit tests for issue phalcon#13058
Update CHANGELOG-3.4.md
Fixed an issue with unit test.
niden pushed a commit that referenced this issue Nov 12, 2018
Wrote unit tests for issue #13058
Update CHANGELOG-3.4.md
Fixed an issue with unit test.
@CameronHall
Copy link
Contributor

This should be fixed in 3.4.2 :)

@niden
Copy link
Member

niden commented Nov 14, 2018

Closing. Thanks @CameronHall

@niden niden closed this as completed Nov 14, 2018
@niden niden added bug A bug report status: low Low and removed Bug - Low labels Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug report status: low Low
Projects
None yet
Development

No branches or pull requests

5 participants