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

deprecate \Cake\Form\Form\errors(); #12601

Merged
merged 4 commits into from
Sep 28, 2018
Merged

Conversation

inoas
Copy link
Contributor

@inoas inoas commented Sep 26, 2018

Ref #12596

@dereuromark
Copy link
Member

dereuromark commented Sep 26, 2018

Afaik the consensus was to focus on the actual functional splitting, and keeping the aliases of existing ones simple, so errors() instead of getErrors() - for the sake of changing it only this would introduce hundreds of more deprecations in the code, wouldnt it?
I am not opposed to it, especially when tools like rector could help to safely migrate.
But also not sure if we have to do that all for 4.0.

Also refs #10930 (which should be closed at this point now IMO).

And there are still actual important things to be done: #9978

@dereuromark dereuromark added this to the 3.7.0 milestone Sep 26, 2018
@@ -244,6 +244,24 @@ public function validate(array $data)
* @return array Last set validation errors.
Copy link
Member

Choose a reason for hiding this comment

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

@deprecated tag needs to be addded in docblock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3.7.0?

@ADmad
Copy link
Member

ADmad commented Sep 26, 2018

Yeah this change can be deemed unnecessary but consistency/intuitiveness in API is also important. Since we have EntityInterface::getErrors() one would intuitively expect to have Form::getErrors().

@inoas
Copy link
Contributor Author

inoas commented Sep 26, 2018

Yes, I wondered why there was not some shared interface.

@ravage84
Copy link
Member

👍 Me and three of my colleagues including @TekkCraft prefer this consistency.

@inoas
Copy link
Contributor Author

inoas commented Sep 27, 2018

I got problems with CI.
Which branch do I need to base my PR on and against which branch should I PR?


deprecated since 3.7.0?

@ADmad
Copy link
Member

ADmad commented Sep 27, 2018

Which branch do I need to base my PR on and against which branch should I PR?

You need to add your changes on top of 3.next and make PR against 3.next (which is already done).

$this->assertSame($entity, $entity->errors('foo', 'bar'));
$this->assertEquals(['bar'], $entity->errors('foo'));
$this->assertEmpty($entity->getErrors());
$this->assertSame($entity, $entity->getErrors('foo', 'bar'));
Copy link
Member

Choose a reason for hiding this comment

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

This should be a setter call $entity->setErrors('foo', 'bar'), that's what's causing tests to fail.

@@ -150,6 +150,18 @@ public function testValidateDeprected()
* @return void
*/
public function testErrors()
{
$this->deprecated(function () {
$this->testGetErrors();
Copy link
Member

Choose a reason for hiding this comment

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

you no need to wrap this method with deprecated when it does not use/have deprecated methods

@inoas
Copy link
Contributor Author

inoas commented Sep 28, 2018

Looks like the appveyor error is unrelated.

Edit: anything else?

@ravage84 ravage84 merged commit df0cfb1 into cakephp:3.next Sep 28, 2018
@ADmad
Copy link
Member

ADmad commented Sep 28, 2018

The 3.next branch of manual also needs to be updated.

@inoas inoas deleted the Form-Form-getErrors branch September 28, 2018 10:04
inoas added a commit to inoas/docs that referenced this pull request Sep 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants