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

No longer return $this on Collection __set() method #4458

Merged

Conversation

neildaniels
Copy link
Contributor

@neildaniels neildaniels commented Jun 30, 2022

Explanation

This PR changes the behavior of __set() methods to no longer return $this. Instead there is no return value.

A return type was never formalized in code, but was defined in the DocBlock for Kirby\Toolkit\Collection.

Although Kirby\Toolkit\Collection returns $this, the subclass Kirby\Cms\Collection does not return anything.

/**
* Internal setter for each object in the Collection.
* This takes care of Component validation and of setting
* the collection prop on each object correctly.
*
* @param string $id
* @param object $object
*/
public function __set(string $id, $object)
{
$this->data[$id] = $object;
}

The Kirby\Cms\Pages class extends Kirby\Cms\Collection and does not override __set() so it therefor also returns nothing.

Kirby\Cms\Structure also extends Kirby\Cms\Collection. It attempts to return a value in its reimplementation of __set(), but because it's returning parent::__set() and the Kirby\Cms\Collection implementation does not return anything, it's effectively returning void.

The formal definition of the magic method __set() has a void return type.

public __set(string $name, mixed $value): void

Additionally, the PHP Docs also say that a return a type on __set() is always ignored. None of their examples return a value either.

Note:
The return value of __set() is ignored because of the way PHP processes the assignment operator. Similarly, __get() is never called when chaining assignments together like this:
$a = $obj->b = 8;

This PR …

Refactoring

  • The $collection->__set() methods consistently no longer return the class instance to match PHP's definition for this method.

Breaking changes

This is technically a breaking change: the return type is ignored, but only for usage such as $collection->foo = $example;. Usage by calling the function literally still would return a value $collection->__set('foo', $example);.

However, the impact of this breaking change should be minimal, considering that Kirby\Cms\Collection never returned a value anyways.

Ready?

  • Unit tests for fixed bug/feature
  • In-code documentation (wherever needed)
  • Tests and checks all pass

For review team

@distantnative
Copy link
Member

@lukasbestle what do you think? maybe this could go into 3.7.1 since it is more like fixing a bug than breaking change?

@lukasbestle lukasbestle added the type: refactoring ♻️ Is about bad code; cleans up code label Jul 3, 2022
@lukasbestle lukasbestle added this to the 3.7.1 milestone Jul 3, 2022
@lukasbestle
Copy link
Member

I've added void type hints for consistency with the formal definition.

@lukasbestle lukasbestle merged commit ebb264e into getkirby:develop Jul 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: refactoring ♻️ Is about bad code; cleans up code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants