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

collection's append() and prepend() behave differently when only one argument is provided #2078

Closed
plagasul opened this issue Sep 12, 2019 · 2 comments

Comments

@plagasul
Copy link

commented Sep 12, 2019

Describe the bug

Note: this issue discusses append() and prepend() methods of collection class, BUT, when following code links from the $page->append() and $page->prepend() reference pages, one leads to 'kirby/src/Cms/Collection.php' while the other leads to 'kirby/src/Toolkit/Collection.php'. I do not know the implications of this, but I assume it is correct, and those are the methods we are using when we do $pages->append($page) or prepend($page)

The methods append() and prepend() from the collection class behave differently when only one argument (for example a page via $pages->append($page)) is provided.

append() checks if the provided element is an object, and if it is, it looks for the object's ID and uses it as key when adding the object to the data array; while prepend() uses array_unshift() effectively adding the element (object or not) without a key, so the key becomes zero (0).

kirby/src/Cms/Collection.php

    /**
     * Appends an element to the data array
     *
     * @param  mixed $key
     * @param  mixed $item
     * @return Kirby\Cms\Collection
     */
    public function append(...$args)
    {
        if (count($args) === 1) {
            if (is_object($args[0]) === true) {
                $this->data[$args[0]->id()] = $args[0];
            } else {
                $this->data[] = $args[0];
            }
        } elseif (count($args) === 2) {
            $this->set($args[0], $args[1]);
        }
        return $this;
    }

kirby/src/Toolkit/Collection.php

    /**
     * Prepends an element to the data array
     *
     * @param  mixed       $key
     * @param  mixed       $item
     * @return self
     */
    public function prepend(...$args)
    {
        if (count($args) === 1) {
            array_unshift($this->data, $args[0]);
        } elseif (count($args) === 2) {
            $data = $this->data;
            $this->data = [];
            $this->set($args[0], $args[1]);
            $this->data += $data;
        }
        return $this;
    }

To Reproduce
Steps to reproduce the behavior:

  1. Append a page to a $pages object, without providing a key, and dump
  2. Prepend a page to a $pages object, without providing a key, and dump
  3. Compare the dumps
  4. Notice in append's dump the key is the page's ID, while in prepend's dump. the key is zero (0)

Expected behavior
Either both work in the same way, checking for an object and looking for its key if it not provided, or neither do, and then possibly throw an Exception, as I believe K2 did.

A possible rewrite of prepend() (bear with my low php skills, please) :

public function prepend(...$args)
{
    if (count($args) === 1) {
        if (is_object($args[0]) === true) {
            $this->prepend($args[0]->id(), $args[0]);
        } else {
            array_unshift($this->data, $args[0]);
        }
    } elseif (count($args) === 2) {
        $data = $this->data;
        $this->data = [];
        $this->set($args[0], $args[1]);
        $this->data += $data;
    }
    return $this;
}

Alternatively this difference could be explained in the reference page of the methods. Right now this difference is not visible unless one checks the code, and it already confused other people than myself, see here.

Kirby Version
"3.1.2-rc.1" but the quoted code is the same in the last version in this repo

Desktop (please complete the following information):

  • OS: Linux Mint cinnaon 19.2

Additional context
Related forum question.

@lukasbestle

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

Your findings regarding the two different implementations in the two different classes are correct. It looks like we implemented a custom Cms implementation for the append() method, but forgot (?) to do the same for prepend().

Your possible solution looks pretty good already, but I think I will do some refactoring to always use the set() method while I'm at it. This also needs some more unit tests.

lukasbestle added a commit that referenced this issue Sep 13, 2019
The methods would currently do absolutely nothing in this case, whereas general PHP behavior with more than the required params is to ignore the superfluous params.

#2078
lukasbestle added a commit that referenced this issue Sep 13, 2019
bastianallgeier added a commit that referenced this issue Sep 16, 2019
The methods would currently do absolutely nothing in this case, whereas general PHP behavior with more than the required params is to ignore the superfluous params.

#2078
bastianallgeier added a commit that referenced this issue Sep 16, 2019
@bastianallgeier

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.