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: Added setExcludedID() to exclude a certain record from uniqueness checking. #1740

Closed
wants to merge 4 commits into from
Closed

BUG: Added setExcludedID() to exclude a certain record from uniqueness checking. #1740

wants to merge 4 commits into from

Conversation

svandragt
Copy link
Contributor

As per issue #1399.

This pull request allows end-users to edit records that contain AjaxUniqueTextFields. Prior to this validation always fails when saving and updated record when AjaxUniqueTextFields are not updated. This is because the validation find the current record in the database.

With the patch, developers can use the new $ajaxutf->setExcludedID($this->ID) to exclude the current record from the uniqueness check.

@svandragt
Copy link
Contributor Author

Travis build failure due to github unavailability, not code.

* Exclude the provided ID in the restrictedTable from uniqueness checking.
* @var [integer]
*/
$this->excludedID = (int)$excludedID;
Copy link
Member

Choose a reason for hiding this comment

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

phpdoc should be placed before function declaration

@chillu
Copy link
Member

chillu commented Apr 9, 2013

Actually, can we change this to use DataList instead of restrictedTable etc? Then you can set the list, with your own exclusions applied (via DataList->exclude())

@chillu
Copy link
Member

chillu commented Apr 9, 2013

So there should be a setList() and getList() functionality. getList() would default to new DataList($this->restrictedTable), and it should throw a deprecation notice if the constructor arg is used. $restrictedField still makes sense, since we can't encapsulate this info in the DataList

@svandragt
Copy link
Contributor Author

OK so this is the new implementation using DataList.
Working example: http://sspaste.com/paste/show/5165743a4851b

Because the ajaxUTF has to add filters / excludes for the current form's submitted values but before validating, I implemented setting a callback (setListProvider) which gets called at the start of the validate() method.

The change to the __constructor is simply that $restrictedField and $restrictedTable are now optional.

Two things:

  1. I'm not sure where the validateURL ever gets used. It gets called in the Field() method but not used as far as I can see.
  2. I've set the Deprecation notice to 3.2, not sure if this is correct.

@ajshort
Copy link
Contributor

ajshort commented Apr 10, 2013

Why not just let setList accept a callback?

*
* @param DataList $list
*/
public function setList(DataList $list) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should accept any SS_List, not just data lists. Also, the API docs aren't very clear - the description isn't great, and the example isn't really necessary.

@chillu
Copy link
Member

chillu commented Apr 10, 2013

Also, please squash this to a single commit and remove the merge commits (with git rebase -i)

$this->validateURL = $validationURL;

$this->restrictedRegex = $restrictedRegex;

parent::__construct($name, $title, $value);
parent::__construct($name, $title, $value, $maxLength);
Copy link
Member

Choose a reason for hiding this comment

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

$maxLength definitely doesn't need to be a new required arg. It shouldn't be a constructor arg at all, we're trying to reduce those rather than add new ones. Can be a standard accessor.

EDIT: OK, this is the parent call ... and TextField has this argument as well (for legacy reasons). Good change.

@chillu
Copy link
Member

chillu commented Jun 2, 2013

Hello Sander, can you please fix the conventions and Andrew's comments on SS_List usage so we can merge?

@wilr
Copy link
Member

wilr commented Jun 2, 2013

@chillu didn't we delete this class from master? Any use changing it if it's deprecated.

@chillu
Copy link
Member

chillu commented Jun 3, 2013

Yeah we have removed it from master in #1974. This pull request and discussion is "older", and targets 3.1. I'm not too fussed with merging it for the sake of making the field slightly less useless in 3.1, but you're right we shouldn't spend much energy on the pull request. Sander, maybe you want to take on this field as your own module?

@simonwelsh
Copy link
Contributor

Issues haven't been addressed in almost a year. Closing.

@simonwelsh simonwelsh closed this Mar 15, 2014
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.

None yet

5 participants