BC mod for saveAll() #894

Merged
merged 1 commit into from Oct 23, 2012

Conversation

Projects
None yet
5 participants
Member

dereuromark commented Oct 9, 2012

while upgrading 1.x to 2.2/2.3:

$this->OfferRequest->set($this->request->data);
$this->OfferRequest->saveAll();

this used to work just fine and would now throw a warning due to missing first argument. which it should not need since we already passed the data in set().

Owner

lorenzo commented Oct 9, 2012

That is a very big change in behavior I rather not do for a minor version release

Member

dereuromark commented Oct 9, 2012

this is supposed to assert BC to how it previously worked. I did not intend any change in behavior. is there a way to allow the saveAll() to be empty again (as it used to be in all previous cake versions)?

$this->OfferRequest->saveAll(array());

does the trick but is pretty hacky (note: you cannot use NULL since it would throw an array_merge warning then).

Member

ceeram commented Oct 9, 2012

How is array() different from no argument?

Member

dereuromark commented Oct 9, 2012

note the difference between saveAll($data) (now) and saveAll($data = null) (used to).
the current behavior forces you to pass a first argument. which in my opinion does not make sense if we set our data using Model::set()

if one doesn't comply a warning is thrown.

Member

ceeram commented Oct 9, 2012

I understand that part, but how would the hacky empty array() solve the issue?

Member

ceeram commented Oct 9, 2012

Nvm, im blind, i misread the default null

Member

dereuromark commented Oct 9, 2012

;)

Member

ADmad commented Oct 10, 2012

There was a similar PR earlier which wasn't applied #646

Member

dereuromark commented Oct 10, 2012

indeed. pretty much the same
the solution would be

public function saveAll($data = array(), $options = array()) {}

to

a) BC comp
b) without breaking the existent behavior

Owner

markstory commented Oct 11, 2012

@lorenzo The current diff doesn't seem like that huge of a change. It would be good to add a test for this, so compatibility isn't broken again in the future.

Owner

lorenzo commented Oct 11, 2012

Yeah, that is what @ceeram told me on IRC. Now that I understand properly how it works I think it is fine to be merged in

Member

dereuromark commented Oct 11, 2012

I will see to it that the test is added

@dereuromark dereuromark comp mod for saveAll()
better approach

test case to assert saveAll still behaves like previous versions
1e2ac0b

markstory merged commit 746eaf9 into cakephp:master Oct 23, 2012

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