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

[Improvement]: Implement $args parameter to Concrete & AbstractObject::save() #13207

Closed
NiklasBr opened this issue Sep 20, 2022 · 0 comments · Fixed by #13222
Closed

[Improvement]: Implement $args parameter to Concrete & AbstractObject::save() #13207

NiklasBr opened this issue Sep 20, 2022 · 0 comments · Fixed by #13222
Assignees
Milestone

Comments

@NiklasBr
Copy link
Contributor

Improvement description

The save() method on these classes actually accept arguments, but since it is by means of func_get_args() rather than method parameters it is not clear from the code overview given by docblock or editor inspections, e.g.

image

Nobody knows, unless reading every single line of code, that it is possible.

The code today:

/**
* @return $this
*
* @throws \Exception
*/
public function save()
{
$isDirtyDetectionDisabled = DataObject::isDirtyDetectionDisabled();
// if the class is newer then better disable the dirty detection. This should fix issues with the query table if
// the inheritance enabled flag has been changed in the meantime
if ($this->getClass()->getModificationDate() >= $this->getModificationDate() && $this->getId()) {
DataObject::disableDirtyDetection();
}
try {
$params = [];
if (func_num_args() && is_array(func_get_arg(0))) {
$params = func_get_arg(0);
}

But if re-written like this:

public function save(...$params) 

Or like this, which is more in line with what #13177 is doing:

public function save($params = []) 

I believe it would be much clearer and the need for calling func_get_args() would not be needed.

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

Successfully merging a pull request may close this issue.

3 participants