Skip to content

Conversation

@dereuromark
Copy link
Member

Docs changes for cakephp/cakephp#9676

This will switch to fluent interface but contains a note for arrays still.
Also the table of possible array keys might have to be switched to setters.

Can we maybe use a table here?

| Setter | Array key | Description |

etc?
Or how can we do it?

@dereuromark dereuromark added this to the 3.x milestone Nov 2, 2016
@andre-pdassis
Copy link
Member

andre-pdassis commented Nov 2, 2016

Currently there is a list of possible keys for each association type in which the description of some key changes a little in each association case.

| Setter | Array key |

Seems good to me, unless we review and unify the descriptions.


}

You can also use arrays to customize your associations::
Copy link
Contributor

Choose a reason for hiding this comment

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

The docs should note that this is deprecated and not recommended.

"For the sake of backwards compatibility you can also use..."

Copy link
Member Author

@dereuromark dereuromark Nov 2, 2016

Choose a reason for hiding this comment

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

It kind of does below it. As of right now it is not deprecated by the way, only less recommended.
And it probably won't either as long as mass assignment as documented on the same page still is around :)

Copy link
Contributor

@inoas inoas Nov 2, 2016

Choose a reason for hiding this comment

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

I'd start the block with that info so that it is clear what you are reading is about out-of-date interfaces.

And why wouldn't it be deprecated? Are there 4 states now: recommended, non-recommended, deprecated, removed? Or can we cut it down to default, deprecated, removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is more a personal preference at this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

So won't the array interface be deprecated for 4.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can decide on that at a later point in time IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Personally I'd prefer to keep the array config style. I know I'm an outlier in that preference though.

@markstory
Copy link
Member

Looks good to me.

@markstory
Copy link
Member

Mentioning the new fluent interface in the migration guide would be good as I use that to do release announcements andcwe recommend that for people who are upgrading.

@dereuromark
Copy link
Member Author

Should we continue here?

Copy link
Contributor

@inoas inoas left a comment

Choose a reason for hiding this comment

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

After the fixes it LGTM.

]);
$this->belongsTo('Users')
->setForeignKey('user_id')
->setJoinType('INNER',);
Copy link
Contributor

Choose a reason for hiding this comment

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

Surplus ','

'dependent' => true,
$this->hasMany('Comments')
->setForeignKey('article_id')
->setDependent(true,
Copy link
Contributor

@inoas inoas Dec 29, 2016

Choose a reason for hiding this comment

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

Surplus ,
Edit: And chain call syntax should end on the same line. Which also has a surplus ].

$this->hasMany('Comments')
->setForeignKey('article_id')
->setDependent(true,
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Surplus ] and ); should be on the line above.

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.

8 participants