eval->required does not work as expected #3277

Closed
Toflar opened this Issue Nov 29, 2011 · 9 comments

Comments

Projects
None yet
3 participants
@Toflar
Member

Toflar commented Nov 29, 2011

Hi Leo

While debugging the Catalog I discovered a bug in DataContainer.php on line 226 (trunk):

$arrData['eval']['required'] = ($this->varValue == '' && $arrData['eval']['mandatory']) ? true : false;

You check for an empty string to determine whether there has been data entered or not.
Unfortunately this leads to a strange behaviour when the following field characteristics are given:

  • the definition of the field is not a string, e.g. "int(10) NOT NULL default '0'"
  • the field is set to mandatory
  • the field has no default value

Then the user doesn't get any asterisk sign because the default value is 0 instead of ''.

Unfortunately I couldn't find any case where this happens in the core for you to reproduce the issue because whenever you have a field with the definition "int(10) NOT NULL default '0'", you provide a default value so $this->varValue in the DataContainer.php is not empty and obviously this is correct because a value is already chosen even if it hasn't been done by the user.

Do you see the problem? =)

Download the attachments
Related issues: #3356

--- Originally created on July 18th, 2011, at 02:00pm (ID 3277)

@ghost ghost assigned leofeyer Nov 29, 2011

@Toflar

This comment has been minimized.

Show comment
Hide comment
@Toflar

Toflar Nov 29, 2011

Member

This would be my proposal how we could solve this problem backwards compatible. This way we leave it to the developer to tell the core when the widget is empty or not.
But still we would maybe need a new eval parameter because how should e.g. the SelectMenu widget know whether '' or 0 means that there hasn't been entered any value at all.
Any other ideas?

--- Originally created on July 22nd, 2011, at 11:04am

Member

Toflar commented Nov 29, 2011

This would be my proposal how we could solve this problem backwards compatible. This way we leave it to the developer to tell the core when the widget is empty or not.
But still we would maybe need a new eval parameter because how should e.g. the SelectMenu widget know whether '' or 0 means that there hasn't been entered any value at all.
Any other ideas?

--- Originally created on July 22nd, 2011, at 11:04am

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Nov 29, 2011

Member

Wouldn't a simple $this->varValue === '' do the job?

--- Originally created on July 27th, 2011, at 10:22am

Member

leofeyer commented Nov 29, 2011

Wouldn't a simple $this->varValue === '' do the job?

--- Originally created on July 27th, 2011, at 10:22am

@Toflar

This comment has been minimized.

Show comment
Hide comment
@Toflar

Toflar Nov 29, 2011

Member

Why should it?

  • NULL would fail
  • '0' would fail
  • 0 would fail
    You would only check whether the value is '' AND of the type string but it can also be an integer, can't it?

--- Originally created on July 27th, 2011, at 11:45am

Member

Toflar commented Nov 29, 2011

Why should it?

  • NULL would fail
  • '0' would fail
  • 0 would fail
    You would only check whether the value is '' AND of the type string but it can also be an integer, can't it?

--- Originally created on July 27th, 2011, at 11:45am

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Nov 29, 2011

Member

You are right. But strlen($this->varValue) should work. It has been changed accidentally and now reset in d089d45.

--- Originally created on July 29th, 2011, at 01:03pm

Member

leofeyer commented Nov 29, 2011

You are right. But strlen($this->varValue) should work. It has been changed accidentally and now reset in d089d45.

--- Originally created on July 29th, 2011, at 01:03pm

@Toflar

This comment has been minimized.

Show comment
Hide comment
@Toflar

Toflar Nov 29, 2011

Member

Hmm, the integer 0 would still fail, because strlen(0) returns 1. Any idea for this?
Could you implement the isEmpty() anyway so we can overwrite it? For example the dcaWizard I wrote together with Andy manages values in child tables and I could check the rowcount in this child table if I had the isEmpty() method =)

--- Originally created on July 29th, 2011, at 01:36pm

Member

Toflar commented Nov 29, 2011

Hmm, the integer 0 would still fail, because strlen(0) returns 1. Any idea for this?
Could you implement the isEmpty() anyway so we can overwrite it? For example the dcaWizard I wrote together with Andy manages values in child tables and I could check the rowcount in this child table if I had the isEmpty() method =)

--- Originally created on July 29th, 2011, at 01:36pm

@aschempp

This comment has been minimized.

Show comment
Hide comment
@aschempp

aschempp Nov 29, 2011

Member

I suppose the correct function would be empty() ?

--- Originally created on July 29th, 2011, at 01:38pm

Member

aschempp commented Nov 29, 2011

I suppose the correct function would be empty() ?

--- Originally created on July 29th, 2011, at 01:38pm

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Nov 29, 2011

Member

Hmm, the integer 0 would still fail, because strlen(0) returns 1. Any idea for this?

This is correct. If you enter 0, you have entered something and thus the field is not mandatory anymore. In contrast, emtpy(0) returns true and that is clearly wrong, because the field is not empty anymore.

--- Originally created on July 29th, 2011, at 03:13pm

Member

leofeyer commented Nov 29, 2011

Hmm, the integer 0 would still fail, because strlen(0) returns 1. Any idea for this?

This is correct. If you enter 0, you have entered something and thus the field is not mandatory anymore. In contrast, emtpy(0) returns true and that is clearly wrong, because the field is not empty anymore.

--- Originally created on July 29th, 2011, at 03:13pm

@Toflar

This comment has been minimized.

Show comment
Hide comment
@Toflar

Toflar Nov 29, 2011

Member

What if the field definition was "NOT NULL default 0"?
Anyway, the isEmpty() method would still be quite useful :)

--- Originally created on July 29th, 2011, at 05:18pm

Member

Toflar commented Nov 29, 2011

What if the field definition was "NOT NULL default 0"?
Anyway, the isEmpty() method would still be quite useful :)

--- Originally created on July 29th, 2011, at 05:18pm

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Nov 29, 2011

Member

--- Originally completed on July 29th, 2011, at 01:03pm

Member

leofeyer commented Nov 29, 2011

--- Originally completed on July 29th, 2011, at 01:03pm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment