Completed
Pull Request — master (#482)
by Roman
29:46
created

ShoppingCart_Controller   D

Complexity

Total Complexity 52

Size/Duplication

Total Lines 282
Duplicated Lines 0 %

Coupling/Cohesion

Components 1
Dependencies 17

Test Coverage

Coverage 71.07%

Importance

Changes 4
Bugs 1 Features 1
Metric Value
wmc 52
c 4
b 1
f 1
lcom 1
cbo 17
dl 0
loc 282
ccs 86
cts 121
cp 0.7107
rs 4.9781

17 Methods

Rating   Name   Duplication   Size   Complexity  
A add_item_link() 0 4 1
A remove_item_link() 0 4 1
A remove_all_item_link() 0 4 1
A set_quantity_item_link() 0 4 1
A params_to_get_string() 0 8 2
A direct() 0 13 4
A init() 0 5 1
B build_url() 0 14 5
D buyableFromRequest() 0 38 10
A add() 0 14 4
A remove() 0 10 3
A removeall() 0 10 3
A setquantity() 0 12 3
A clear() 0 7 2
A index() 0 10 3
A debug() 0 14 4
A updateLocale() 0 7 4

How to fix   Complexity   

Complex Class

Complex classes like ShoppingCart_Controller often do a lot of different things. To break such a class down, we need to identify a cohesive component within that class. A common approach to find such a component is to look for fields/methods that share the same prefixes, or suffixes. You can also have a look at the cohesion graph to spot any un-connected, or weakly-connected components.

Once you have determined the fields that belong together, you can apply the Extract Class refactoring. If the component makes sense as a sub-class, Extract Subclass is also a candidate, and is often faster.

While breaking up the class, it is a good idea to analyze how other classes use ShoppingCart_Controller, and based on these observations, apply Extract Interface, too.

1
<?php
2
3
/**
4
 * Encapsulated manipulation of the current order using a singleton pattern.
5
 *
6
 * Ensures that an order is only started (persisted to DB) when necessary,
7
 * and all future changes are on the same order, until the order has is placed.
8
 * The requirement for starting an order is to adding an item to the cart.
9
 *
10
 * @package shop
11
 */
12
class ShoppingCart
0 ignored issues
show
Complexity introduced by
This class has a complexity of 50 which exceeds the configured maximum of 50.

The class complexity is the sum of the complexity of all methods. A very high value is usually an indication that your class does not follow the single reponsibility principle and does more than one job.

Some resources for further reading:

You can also find more detailed suggestions for refactoring in the “Code” section of your repository.

Loading history...
13
{
14
    private static $cartid_session_name = 'shoppingcartid';
15
16
    private        $order;
17
18
    private        $calculateonce       = false;
19
20
    private        $message;
21
22
    private        $type;
23
24
    private static $instance;
25
26
    /**
27
     * Access for only allowing access to one (singleton) ShoppingCart.
28
     *
29
     * @return ShoppingCart
30
     */
31 90
    public static function singleton()
32
    {
33 90
        if (self::$instance === null) {
34 2
            self::$instance = new ShoppingCart();
35 2
        }
36
37 90
        return self::$instance;
38
    }
39
40
    /**
41
     * Shortened alias for ShoppingCart::singleton()->current()
42
     *
43
     * @return Order
44
     */
45 16
    public static function curr()
46
    {
47 16
        return self::singleton()->current();
48
    }
49
50
    /**
51
     * Singleton prevents constructing a ShoppingCart any other way.
52
     */
53 2
    private function __construct()
54
    {
55 2
    }
56
57
    /**
58
     * Get the current order, or return null if it doesn't exist.
59
     *
60
     * @return Order
61
     */
62 90
    public function current()
63
    {
64
        //find order by id saved to session (allows logging out and retaining cart contents)
65 90
        if (!$this->order && $sessionid = Session::get(self::$cartid_session_name)) {
66
            $this->order = Order::get()->filter(
67
                array(
68
                    "Status" => "Cart",
69
                    "ID"     => $sessionid,
70
                )
71
            )->first();
72
        }
73 90
        if (!$this->calculateonce && $this->order) {
74 2
            $this->order->calculate();
75 4
            $this->calculateonce = true;
76 18
        }
77
78 90
        return $this->order ? $this->order : false;
79
    }
80
81
    /**
82
     * Set the current cart
83
     *
84
     * @param Order
85
     *
86
     * @return ShoppingCart
87
     */
88 7
    public function setCurrent(Order $cart)
89
    {
90 7
        if (!$cart->IsCart()) {
91
            trigger_error("Passed Order object is not cart status", E_ERROR);
92
        }
93 7
        $this->order = $cart;
94 7
        Session::set(self::$cartid_session_name, $cart->ID);
95
96 7
        return $this;
97
    }
98
99
    /**
100
     * Helper that only allows orders to be started internally.
101
     *
102
     * @return Order
103
     */
104 17
    protected function findOrMake()
105
    {
106 17
        if ($this->current()) {
107 17
            return $this->current();
108
        }
109 9
        $this->order = Order::create();
110 9
        if (Member::config()->login_joins_cart && Member::currentUserID()) {
111 2
            $this->order->MemberID = Member::currentUserID();
0 ignored issues
show
Documentation introduced by
The property MemberID does not exist on object<Order>. Since you implemented __set, maybe consider adding a @property annotation.

Since your code implements the magic setter _set, this function will be called for any write access on an undefined variable. You can add the @property annotation to your class or interface to document the existence of this variable.

<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}

Since the property has write access only, you can use the @property-write annotation instead.

Of course, you may also just have mistyped another name, in which case you should fix the error.

See also the PhpDoc documentation for @property.

Loading history...
112 2
        }
113 9
        $this->order->write();
114 9
        $this->order->extend('onStartOrder');
115 9
        Session::set(self::$cartid_session_name, $this->order->ID);
116
117 9
        return $this->order;
118
    }
119
120
    /**
121
     * Adds an item to the cart
122
     *
123
     * @param Buyable $buyable
124
     * @param number  $quantity
125
     * @param unknown $filter
126
     *
127
     * @return boolean|OrderItem false or the new/existing item
128
     */
129 16
    public function add(Buyable $buyable, $quantity = 1, $filter = array())
130
    {
131 16
        $order = $this->findOrMake();
132 16
        $order->extend("beforeAdd", $buyable, $quantity, $filter);
133 16
        if (!$buyable) {
134
135
            return $this->error(_t("ShoppingCart.PRODUCTNOTFOUND", "Product not found."));
136
        }
137 16
        $item = $this->findOrMakeItem($buyable, $filter);
138 16
        if (!$item) {
139
140 1
            return false;
141
        }
142 16
        if (!$item->_brandnew) {
0 ignored issues
show
Documentation introduced by
The property _brandnew does not exist on object<OrderItem>. Since you implemented __get, maybe consider adding a @property annotation.

Since your code implements the magic getter _get, this function will be called for any read access on an undefined variable. You can add the @property annotation to your class or interface to document the existence of this variable.

<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}

If the property has read access only, you can use the @property-read annotation instead.

Of course, you may also just have mistyped another name, in which case you should fix the error.

See also the PhpDoc documentation for @property.

Loading history...
143 3
            $item->Quantity += $quantity;
0 ignored issues
show
Documentation introduced by
The property Quantity does not exist on object<OrderItem>. Since you implemented __set, maybe consider adding a @property annotation.

Since your code implements the magic setter _set, this function will be called for any write access on an undefined variable. You can add the @property annotation to your class or interface to document the existence of this variable.

<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}

Since the property has write access only, you can use the @property-write annotation instead.

Of course, you may also just have mistyped another name, in which case you should fix the error.

See also the PhpDoc documentation for @property.

Loading history...
144 3
        } else {
145 16
            $item->Quantity = $quantity;
0 ignored issues
show
Documentation introduced by
The property Quantity does not exist on object<OrderItem>. Since you implemented __set, maybe consider adding a @property annotation.

Since your code implements the magic setter _set, this function will be called for any write access on an undefined variable. You can add the @property annotation to your class or interface to document the existence of this variable.

<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}

Since the property has write access only, you can use the @property-write annotation instead.

Of course, you may also just have mistyped another name, in which case you should fix the error.

See also the PhpDoc documentation for @property.

Loading history...
146
        }
147 16
        $item->write();
148 16
        $order->extend("afterAdd", $item, $buyable, $quantity, $filter);
149 16
        $this->message(_t("ShoppingCart.ITEMADD", "Item has been added successfully."));
150
151 16
        return $item;
152
    }
153
154
    /**
155
     * Remove an item from the cart.
156
     *
157
     * @param     id        or Buyable $buyable
158
     * @param     $item
159
     * @param int $quantity - number of items to remove, or leave null for all items (default)
160
     *
161
     * @return boolean success/failure
162
     */
163 4
    public function remove(Buyable $buyable, $quantity = null, $filter = array())
164
    {
165 4
        $order = $this->current();
166
167 4
        if (!$order) {
168
            return $this->error(_t("ShoppingCart.NOORDER", "No current order."));
169
        }
170
171 4
        $order->extend("beforeRemove", $buyable, $quantity, $filter);
172
173 4
        $item = $this->get($buyable, $filter);
174
175 4
        if (!$item) {
176 1
            return false;
177
        }
178
179
        //if $quantity will become 0, then remove all
180 4
        if (!$quantity || ($item->Quantity - $quantity) <= 0) {
181 4
            $item->delete();
182 4
            $item->destroy();
183 4
        } else {
184 1
            $item->Quantity -= $quantity;
185 1
            $item->write();
186
        }
187 4
        $order->extend("afterRemove", $item, $buyable, $quantity, $filter);
188 4
        $this->message(_t("ShoppingCart.ITEMREMOVED", "Item has been successfully removed."));
189
190 4
        return true;
191
    }
192
193
    /**
194
     * Sets the quantity of an item in the cart.
195
     * Will automatically add or remove item, if necessary.
196
     *
197
     * @param     id or Buyable $buyable
198
     * @param     $item
199
     * @param int $quantity
200
     *
201
     * @return boolean|OrderItem false or the new/existing item
202
     */
203 3
    public function setQuantity(Buyable $buyable, $quantity = 1, $filter = array())
204
    {
205 3
        if ($quantity <= 0) {
206
            return $this->remove($buyable, $quantity, $filter);
207
        }
208 3
        $order = $this->findOrMake();
209 3
        $item = $this->findOrMakeItem($buyable, $filter);
210 3
        if (!$item) {
211
212
            return false;
213 3
        }
214 3
        $order->extend("beforeSetQuantity", $buyable, $quantity, $filter);
215 3
        $item->Quantity = $quantity;
0 ignored issues
show
Documentation introduced by
The property Quantity does not exist on object<OrderItem>. Since you implemented __set, maybe consider adding a @property annotation.

Since your code implements the magic setter _set, this function will be called for any write access on an undefined variable. You can add the @property annotation to your class or interface to document the existence of this variable.

<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}

Since the property has write access only, you can use the @property-write annotation instead.

Of course, you may also just have mistyped another name, in which case you should fix the error.

See also the PhpDoc documentation for @property.

Loading history...
216 3
        $item->write();
217 3
        $order->extend("afterSetQuantity", $item, $buyable, $quantity, $filter);
218 3
        $this->message(_t("ShoppingCart.QUANTITYSET", "Quantity has been set."));
219
220 3
        return $item;
221
    }
222
223
    /**
224
     * Finds or makes an order item for a given product + filter.
225
     *
226
     * @param        id or Buyable $buyable
227
     * @param string $filter
228
     *
229
     * @return OrderItem the found or created item
230
     */
231 17
    private function findOrMakeItem(Buyable $buyable, $filter = array())
232
    {
233 17
        $order = $this->findOrMake();
234
235 17
        if (!$buyable || !$order) {
236
            return false;
0 ignored issues
show
Bug Best Practice introduced by
The return type of return false; (false) is incompatible with the return type documented by ShoppingCart::findOrMakeItem of type OrderItem.

If you return a value from a function or method, it should be a sub-type of the type that is given by the parent type f.e. an interface, or abstract method. This is more formally defined by the Lizkov substitution principle, and guarantees that classes that depend on the parent type can use any instance of a child type interchangably. This principle also belongs to the SOLID principles for object oriented design.

Let’s take a look at an example:

class Author {
    private $name;

    public function __construct($name) {
        $this->name = $name;
    }

    public function getName() {
        return $this->name;
    }
}

abstract class Post {
    public function getAuthor() {
        return 'Johannes';
    }
}

class BlogPost extends Post {
    public function getAuthor() {
        return new Author('Johannes');
    }
}

class ForumPost extends Post { /* ... */ }

function my_function(Post $post) {
    echo strtoupper($post->getAuthor());
}

Our function my_function expects a Post object, and outputs the author of the post. The base class Post returns a simple string and outputting a simple string will work just fine. However, the child class BlogPost which is a sub-type of Post instead decided to return an object, and is therefore violating the SOLID principles. If a BlogPost were passed to my_function, PHP would not complain, but ultimately fail when executing the strtoupper call in its body.

Loading history...
237
        }
238
239 17
        $item = $this->get($buyable, $filter);
0 ignored issues
show
Bug introduced by
It seems like $filter defined by parameter $filter on line 231 can also be of type string; however, ShoppingCart::get() does only seem to accept array, maybe add an additional type check?

This check looks at variables that have been passed in as parameters and are passed out again to other methods.

If the outgoing method call has stricter type requirements than the method itself, an issue is raised.

An additional type check may prevent trouble.

Loading history...
Bug Compatibility introduced by
The expression $this->get($buyable, $filter); of type the adds the type the to the return on line 266 which is incompatible with the return type documented by ShoppingCart::findOrMakeItem of type OrderItem.
Loading history...
240
241 17
        if (!$item) {
242 17
            $member = Member::currentUser();
243
244 17
            if (!$buyable->canPurchase($member)) {
245 1
                return $this->error(
0 ignored issues
show
Bug Best Practice introduced by
The return type of return $this->error(spri...18n_singular_name()))); (boolean) is incompatible with the return type documented by ShoppingCart::findOrMakeItem of type OrderItem.

If you return a value from a function or method, it should be a sub-type of the type that is given by the parent type f.e. an interface, or abstract method. This is more formally defined by the Lizkov substitution principle, and guarantees that classes that depend on the parent type can use any instance of a child type interchangably. This principle also belongs to the SOLID principles for object oriented design.

Let’s take a look at an example:

class Author {
    private $name;

    public function __construct($name) {
        $this->name = $name;
    }

    public function getName() {
        return $this->name;
    }
}

abstract class Post {
    public function getAuthor() {
        return 'Johannes';
    }
}

class BlogPost extends Post {
    public function getAuthor() {
        return new Author('Johannes');
    }
}

class ForumPost extends Post { /* ... */ }

function my_function(Post $post) {
    echo strtoupper($post->getAuthor());
}

Our function my_function expects a Post object, and outputs the author of the post. The base class Post returns a simple string and outputting a simple string will work just fine. However, the child class BlogPost which is a sub-type of Post instead decided to return an object, and is therefore violating the SOLID principles. If a BlogPost were passed to my_function, PHP would not complain, but ultimately fail when executing the strtoupper call in its body.

Loading history...
246 1
                    sprintf(
247 1
                        _t(
248 1
                            "ShoppingCart.CANNOTPURCHASE",
249
                            "This %s cannot be purchased."
250 1
                        ),
251 1
                        strtolower($buyable->i18n_singular_name())
0 ignored issues
show
Bug introduced by
It seems like you code against a concrete implementation and not the interface Buyable as the method i18n_singular_name() does only exist in the following implementations of said interface: CustomProduct, ProductVariation.

Let’s take a look at an example:

interface User
{
    /** @return string */
    public function getPassword();
}

class MyUser implements User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different implementation of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. Add an additional type-check:

    class AuthSystem
    {
        public function authenticate(User $user)
        {
            if ($user instanceof MyUser) {
                $this->logger->info(/** ... */);
            }
    
            // or alternatively
            if ( ! $user instanceof MyUser) {
                throw new \LogicException(
                    '$user must be an instance of MyUser, '
                   .'other instances are not supported.'
                );
            }
    
        }
    }
    
Note: PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case.
  1. Add the method to the interface:

    interface User
    {
        /** @return string */
        public function getPassword();
    
        /** @return string */
        public function getDisplayName();
    }
    
Loading history...
252 1
                    )
253 1
                );
254
                //TODO: produce a more specific message
255
            }
256
257 17
            $item = $buyable->createItem(1, $filter);
0 ignored issues
show
Bug introduced by
It seems like $filter defined by parameter $filter on line 231 can also be of type string; however, Buyable::createItem() does only seem to accept array, maybe add an additional type check?

This check looks at variables that have been passed in as parameters and are passed out again to other methods.

If the outgoing method call has stricter type requirements than the method itself, an issue is raised.

An additional type check may prevent trouble.

Loading history...
258 17
            $item->OrderID = $order->ID;
0 ignored issues
show
Documentation introduced by
The property OrderID does not exist on object<OrderItem>. Since you implemented __set, maybe consider adding a @property annotation.

Since your code implements the magic setter _set, this function will be called for any write access on an undefined variable. You can add the @property annotation to your class or interface to document the existence of this variable.

<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}

Since the property has write access only, you can use the @property-write annotation instead.

Of course, you may also just have mistyped another name, in which case you should fix the error.

See also the PhpDoc documentation for @property.

Loading history...
259 17
            $item->write();
260
261 17
            $order->Items()->add($item);
0 ignored issues
show
Documentation Bug introduced by
The method Items does not exist on object<Order>? Since you implemented __call, maybe consider adding a @method annotation.

If you implement __call and you know which methods are available, you can improve IDE auto-completion and static analysis by adding a @method annotation to the class.

This is often the case, when __call is implemented by a parent class and only the child class knows which methods exist:

class ParentClass {
    private $data = array();

    public function __call($method, array $args) {
        if (0 === strpos($method, 'get')) {
            return $this->data[strtolower(substr($method, 3))];
        }

        throw new \LogicException(sprintf('Unsupported method: %s', $method));
    }
}

/**
 * If this class knows which fields exist, you can specify the methods here:
 *
 * @method string getName()
 */
class SomeClass extends ParentClass { }
Loading history...
262
263 17
            $item->_brandnew = true; // flag as being new
0 ignored issues
show
Documentation introduced by
The property _brandnew does not exist on object<OrderItem>. Since you implemented __set, maybe consider adding a @property annotation.

Since your code implements the magic setter _set, this function will be called for any write access on an undefined variable. You can add the @property annotation to your class or interface to document the existence of this variable.

<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}

Since the property has write access only, you can use the @property-write annotation instead.

Of course, you may also just have mistyped another name, in which case you should fix the error.

See also the PhpDoc documentation for @property.

Loading history...
264 17
        }
265
266 17
        return $item;
267
    }
268
269
    /**
270
     * Finds an existing order item.
271
     *
272
     * @param Buyable $buyable
273
     * @param string  $filter
0 ignored issues
show
Documentation introduced by
There is no parameter named $filter. Did you maybe mean $customfilter?

This check looks for PHPDoc comments describing methods or function parameters that do not exist on the corresponding method or function. It has, however, found a similar but not annotated parameter which might be a good fit.

Consider the following example. The parameter $ireland is not defined by the method finale(...).

/**
 * @param array $germany
 * @param array $ireland
 */
function finale($germany, $island) {
    return "2:1";
}

The most likely cause is that the parameter was changed, but the annotation was not.

Loading history...
274
     *
275
     * @return the item requested, or false
276
     */
277 21
    public function get(Buyable $buyable, $customfilter = array())
278 14
    {
279 21
        $order = $this->current();
280 20
        if (!$buyable || !$order) {
281
            return false;
0 ignored issues
show
Bug Best Practice introduced by
The return type of return false; (false) is incompatible with the return type documented by ShoppingCart::get of type the.

If you return a value from a function or method, it should be a sub-type of the type that is given by the parent type f.e. an interface, or abstract method. This is more formally defined by the Lizkov substitution principle, and guarantees that classes that depend on the parent type can use any instance of a child type interchangably. This principle also belongs to the SOLID principles for object oriented design.

Let’s take a look at an example:

class Author {
    private $name;

    public function __construct($name) {
        $this->name = $name;
    }

    public function getName() {
        return $this->name;
    }
}

abstract class Post {
    public function getAuthor() {
        return 'Johannes';
    }
}

class BlogPost extends Post {
    public function getAuthor() {
        return new Author('Johannes');
    }
}

class ForumPost extends Post { /* ... */ }

function my_function(Post $post) {
    echo strtoupper($post->getAuthor());
}

Our function my_function expects a Post object, and outputs the author of the post. The base class Post returns a simple string and outputting a simple string will work just fine. However, the child class BlogPost which is a sub-type of Post instead decided to return an object, and is therefore violating the SOLID principles. If a BlogPost were passed to my_function, PHP would not complain, but ultimately fail when executing the strtoupper call in its body.

Loading history...
282
        }
283
        $filter = array(
284 20
            'OrderID' => $order->ID,
285 20
        );
286 20
        $itemclass = Config::inst()->get(get_class($buyable), 'order_item');
287 20
        $relationship = Config::inst()->get($itemclass, 'buyable_relationship');
288 20
        $filter[$relationship . "ID"] = $buyable->ID;
0 ignored issues
show
Bug introduced by
Accessing ID on the interface Buyable suggest that you code against a concrete implementation. How about adding an instanceof check?

If you access a property on an interface, you most likely code against a concrete implementation of the interface.

Available Fixes

  1. Adding an additional type check:

    interface SomeInterface { }
    class SomeClass implements SomeInterface {
        public $a;
    }
    
    function someFunction(SomeInterface $object) {
        if ($object instanceof SomeClass) {
            $a = $object->a;
        }
    }
    
  2. Changing the type hint:

    interface SomeInterface { }
    class SomeClass implements SomeInterface {
        public $a;
    }
    
    function someFunction(SomeClass $object) {
        $a = $object->a;
    }
    
Loading history...
289 20
        $required = array('Order', $relationship);
290 20
        if (is_array($itemclass::config()->required_fields)) {
291 20
            $required = array_merge($required, $itemclass::config()->required_fields);
292 20
        }
293 20
        $query = new MatchObjectFilter($itemclass, array_merge($customfilter, $filter), $required);
294 20
        $item = $itemclass::get()->where($query->getFilter())->first();
295 20
        if (!$item) {
296 20
            return $this->error(_t("ShoppingCart.ITEMNOTFOUND", "Item not found."));
0 ignored issues
show
Bug Best Practice introduced by
The return type of return $this->error(_t('...', 'Item not found.')); (boolean) is incompatible with the return type documented by ShoppingCart::get of type the.

If you return a value from a function or method, it should be a sub-type of the type that is given by the parent type f.e. an interface, or abstract method. This is more formally defined by the Lizkov substitution principle, and guarantees that classes that depend on the parent type can use any instance of a child type interchangably. This principle also belongs to the SOLID principles for object oriented design.

Let’s take a look at an example:

class Author {
    private $name;

    public function __construct($name) {
        $this->name = $name;
    }

    public function getName() {
        return $this->name;
    }
}

abstract class Post {
    public function getAuthor() {
        return 'Johannes';
    }
}

class BlogPost extends Post {
    public function getAuthor() {
        return new Author('Johannes');
    }
}

class ForumPost extends Post { /* ... */ }

function my_function(Post $post) {
    echo strtoupper($post->getAuthor());
}

Our function my_function expects a Post object, and outputs the author of the post. The base class Post returns a simple string and outputting a simple string will work just fine. However, the child class BlogPost which is a sub-type of Post instead decided to return an object, and is therefore violating the SOLID principles. If a BlogPost were passed to my_function, PHP would not complain, but ultimately fail when executing the strtoupper call in its body.

Loading history...
297
        }
298
299 11
        return $item;
300
    }
301
302
    /**
303
     * Store old cart id in session order history
304
     */
305 1
    public function archiveorderid()
306
    {
307 1
        $order = Order::get()
308
            ->filter("Status:not", "Cart")
309
            ->byId(Session::get(self::$cartid_session_name));
310
        if ($order && !$order->IsCart()) {
311
            OrderManipulation::add_session_order($order);
0 ignored issues
show
Compatibility introduced by
$order of type object<DataObject> is not a sub-type of object<Order>. It seems like you assume a child class of the class DataObject to be always present.

This check looks for parameters that are defined as one type in their type hint or doc comment but seem to be used as a narrower type, i.e an implementation of an interface or a subclass.

Consider changing the type of the parameter or doing an instanceof check before assuming your parameter is of the expected type.

Loading history...
312
        }
313
        $this->clear();
314
    }
315
316
    /**
317
     * Empty / abandon the entire cart.
318
     *
319
     * @return bool - true if successful, false if no cart found
320
     */
321 10
    public function clear()
322
    {
323 10
        Session::clear(self::$cartid_session_name);
324 10
        $order = $this->current();
325 10
        $this->order = null;
326 10
        if (!$order) {
327 4
            return $this->error(_t("ShoppingCart.NOCARTFOUND", "No cart found."));
328
        }
329 9
        $order->write();
330 9
        $this->message(_t("ShoppingCart.CLEARED", "Cart was successfully cleared."));
331
332 9
        return true;
333
    }
334
335
    /**
336
     * Store a new error.
337
     */
338 21
    protected function error($message)
339
    {
340 21
        $this->message($message, "bad");
341
342 21
        return false;
343
    }
344
345
    /**
346
     * Store a message to be fed back to user.
347
     *
348
     * @param string $message
349
     * @param string $type - good, bad, warning
350
     */
351 22
    protected function message($message, $type = "good")
352
    {
353 22
        $this->message = $message;
354 22
        $this->type = $type;
355 22
    }
356
357 14
    public function getMessage()
358 14
    {
359
        return $this->message;
360
    }
361
362
    public function getMessageType()
363
    {
364
        return $this->type;
365
    }
366
367
    public function clearMessage()
368
    {
369
        $this->message = null;
370
    }
371
372
    //singleton protection
373
    public function __clone()
374
    {
375
        trigger_error('Clone is not allowed.', E_USER_ERROR);
376
    }
377
378
    public function __wakeup()
379
    {
380
        trigger_error('Unserializing is not allowed.', E_USER_ERROR);
381
    }
382
}
383
384
/**
385
 * Manipulate the cart via urls.
386
 */
387
class ShoppingCart_Controller extends Controller
0 ignored issues
show
Complexity introduced by
This class has a complexity of 52 which exceeds the configured maximum of 50.

The class complexity is the sum of the complexity of all methods. A very high value is usually an indication that your class does not follow the single reponsibility principle and does more than one job.

Some resources for further reading:

You can also find more detailed suggestions for refactoring in the “Code” section of your repository.

Loading history...
388
{
389
    private static $url_segment         = "shoppingcart";
390
391
    private static $direct_to_cart_page = false;
392
393
    protected      $cart;
394
395
    private static $url_handlers        = array(
0 ignored issues
show
Comprehensibility introduced by
Consider using a different property name as you override a private property of the parent class.
Loading history...
396
        '$Action/$Buyable/$ID' => 'handleAction',
397
    );
398
399
    private static $allowed_actions     = array(
0 ignored issues
show
Comprehensibility introduced by
Consider using a different property name as you override a private property of the parent class.
Loading history...
400
        'add',
401
        'additem',
402
        'remove',
403
        'removeitem',
404
        'removeall',
405
        'removeallitem',
406
        'setquantity',
407
        'setquantityitem',
408
        'clear',
409
        'debug',
410
    );
411
412 5
    public static function add_item_link(Buyable $buyable, $parameters = array())
413
    {
414 5
        return self::build_url("add", $buyable, $parameters);
415
    }
416
417 2
    public static function remove_item_link(Buyable $buyable, $parameters = array())
418
    {
419 2
        return self::build_url("remove", $buyable, $parameters);
420
    }
421
422 2
    public static function remove_all_item_link(Buyable $buyable, $parameters = array())
423
    {
424 2
        return self::build_url("removeall", $buyable, $parameters);
425
    }
426
427 3
    public static function set_quantity_item_link(Buyable $buyable, $parameters = array())
428
    {
429 3
        return self::build_url("setquantity", $buyable, $parameters);
430
    }
431
432
    /**
433
     * Helper for creating a url
434
     */
435 5
    protected static function build_url($action, $buyable, $params = array())
436
    {
437 5
        if (!$action || !$buyable) {
438
            return false;
439
        }
440 5
        if (SecurityToken::is_enabled() && !self::config()->disable_security_token) {
441
            $params[SecurityToken::inst()->getName()] = SecurityToken::inst()->getValue();
442
        }
443 5
        return self::config()->url_segment . '/' .
444 5
        $action . '/' .
445 5
        $buyable->class . "/" .
446 5
        $buyable->ID .
447 5
        self::params_to_get_string($params);
448
    }
449
450
    /**
451
     * Creates the appropriate string parameters for links from array
452
     *
453
     * Produces string such as: MyParam%3D11%26OtherParam%3D1
454
     *     ...which decodes to: MyParam=11&OtherParam=1
455
     *
456
     * you will need to decode the url with javascript before using it.
457
     */
458 5
    protected static function params_to_get_string($array)
459
    {
460 5
        if ($array & count($array > 0)) {
461 2
            array_walk($array, create_function('&$v,$k', '$v = $k."=".$v ;'));
462 2
            return "?" . implode("&", $array);
463
        }
464 5
        return "";
465
    }
466
467
    /**
468
     * This is used here and in VariationForm and AddProductForm
469
     *
470
     * @param bool|string $status
471
     *
472
     * @return bool
473
     */
474 3
    public static function direct($status = true)
475
    {
476 3
        if (Director::is_ajax()) {
477
            return $status;
478
        }
479 3
        if (self::config()->direct_to_cart_page && $cartlink = CartPage::find_link()) {
480
            Controller::curr()->redirect($cartlink);
481
            return;
482
        } else {
483 3
            Controller::curr()->redirectBack();
484 3
            return;
485
        }
486
    }
487
488 3
    public function init()
489
    {
490 3
        parent::init();
491 3
        $this->cart = ShoppingCart::singleton();
492 3
    }
493
494
    /**
495
     * @return Product|ProductVariation|Buyable
496
     */
497 3
    protected function buyableFromRequest()
0 ignored issues
show
Complexity introduced by
This operation has 480 execution paths which exceeds the configured maximum of 200.

A high number of execution paths generally suggests many nested conditional statements and make the code less readible. This can usually be fixed by splitting the method into several smaller methods.

You can also find more information in the “Code” section of your repository.

Loading history...
498
    {
499 3
        $request = $this->getRequest();
500 3
        if (
501
            SecurityToken::is_enabled() &&
502
            !self::config()->disable_security_token &&
503
            !SecurityToken::inst()->checkRequest($request)
504
        ) {
505
            return $this->httpError(
506 3
                400,
507 3
                _t("ShoppingCart.CSRF", "Invalid security token, possible CSRF attack.")
508
            );
509
        }
510
        $id = (int)$request->param('ID');
511 3
        if (empty($id)) {
512 3
            //TODO: store error message
513 3
            return null;
514 3
        }
515 3
        $buyableclass = "Product";
516
        if ($class = $request->param('Buyable')) {
517
            $buyableclass = Convert::raw2sql($class);
518
        }
519
        if (!ClassInfo::exists($buyableclass)) {
520 3
            //TODO: store error message
521
            return null;
522 3
        }
523 3
        //ensure only live products are returned, if they are versioned
524 3
        $buyable = Object::has_extension($buyableclass, 'Versioned')
0 ignored issues
show
Bug introduced by
It seems like $buyableclass defined by \Convert::raw2sql($class) on line 517 can also be of type array; however, Object::has_extension() does only seem to accept string, maybe add an additional type check?

If a method or function can return multiple different values and unless you are sure that you only can receive a single value in this context, we recommend to add an additional type check:

/**
 * @return array|string
 */
function returnsDifferentValues($x) {
    if ($x) {
        return 'foo';
    }

    return array();
}

$x = returnsDifferentValues($y);
if (is_array($x)) {
    // $x is an array.
}

If this a common case that PHP Analyzer should handle natively, please let us know by opening an issue.

Loading history...
525 3
            ?
526
            Versioned::get_by_stage($buyableclass, 'Live')->byID($id)
0 ignored issues
show
Bug introduced by
It seems like $buyableclass defined by \Convert::raw2sql($class) on line 517 can also be of type array; however, Versioned::get_by_stage() does only seem to accept string, maybe add an additional type check?

If a method or function can return multiple different values and unless you are sure that you only can receive a single value in this context, we recommend to add an additional type check:

/**
 * @return array|string
 */
function returnsDifferentValues($x) {
    if ($x) {
        return 'foo';
    }

    return array();
}

$x = returnsDifferentValues($y);
if (is_array($x)) {
    // $x is an array.
}

If this a common case that PHP Analyzer should handle natively, please let us know by opening an issue.

Loading history...
527 1
            :
528
            DataObject::get($buyableclass)->byID($id);
0 ignored issues
show
Bug introduced by
It seems like $buyableclass defined by \Convert::raw2sql($class) on line 517 can also be of type array; however, DataObject::get() does only seem to accept string|null, maybe add an additional type check?

If a method or function can return multiple different values and unless you are sure that you only can receive a single value in this context, we recommend to add an additional type check:

/**
 * @return array|string
 */
function returnsDifferentValues($x) {
    if ($x) {
        return 'foo';
    }

    return array();
}

$x = returnsDifferentValues($y);
if (is_array($x)) {
    // $x is an array.
}

If this a common case that PHP Analyzer should handle natively, please let us know by opening an issue.

Loading history...
529 3
        if (!$buyable || !($buyable instanceof Buyable)) {
530
            //TODO: store error message
531
            return null;
532
        }
533
        return $buyable;
534
    }
535
536
    /**
537
     * Action: add item to cart
538
     *
539 10
     * @param SS_HTTPRequest $request
540
     *
541 3
     * @return SS_HTTPResponse
542 10
     */
543 3
    public function add($request)
544 3
    {
545 3
        if ($product = $this->buyableFromRequest()) {
546 3
            $quantity = (int)$request->getVar('quantity');
547 3
            if (!$quantity) {
548
                $quantity = 1;
549 3
            }
550 3
            $this->cart->add($product, $quantity, $request->getVars());
551 3
        }
552
553
        $this->updateLocale($request);
554
        $this->extend('updateAddResponse', $request, $response, $product, $quantity);
555
        return $response ? $response : self::direct();
556
    }
557
558
    /**
559
     * Action: remove a certain number of items from the cart
560
     *
561 1
     * @param SS_HTTPRequest $request
562
     *
563 1
     * @return SS_HTTPResponse
564 1
     */
565 1
    public function remove($request)
566
    {
567 1
        if ($product = $this->buyableFromRequest()) {
568 1
            $this->cart->remove($product, $quantity = 1, $request->getVars());
569 1
        }
570
571
        $this->updateLocale($request);
572
        $this->extend('updateRemoveResponse', $request, $response, $product, $quantity);
573
        return $response ? $response : self::direct();
574
    }
575
576
    /**
577
     * Action: remove all of an item from the cart
578
     *
579 1
     * @param SS_HTTPRequest $request
580
     *
581 1
     * @return SS_HTTPResponse
582 1
     */
583 1
    public function removeall($request)
584
    {
585 1
        if ($product = $this->buyableFromRequest()) {
586 1
            $this->cart->remove($product, null, $request->getVars());
587 1
        }
588
589
        $this->updateLocale($request);
590
        $this->extend('updateRemoveAllResponse', $request, $response, $product);
591
        return $response ? $response : self::direct();
592
    }
593
594
    /**
595
     * Action: update the quantity of an item in the cart
596
     *
597 2
     * @param $request
598
     *
599 2
     * @return AjaxHTTPResponse|bool
600 2
     */
601 2
    public function setquantity($request)
602 2
    {
603 2
        $product = $this->buyableFromRequest();
604
        $quantity = (int)$request->getVar('quantity');
605 2
        if ($product) {
606 2
            $this->cart->setQuantity($product, $quantity, $request->getVars());
607 2
        }
608
609
        $this->updateLocale($request);
610
        $this->extend('updateSetQuantityResponse', $request, $response, $product, $quantity);
611
        return $response ? $response : self::direct();
612
    }
613
614
    /**
615
     * Action: clear the cart
616
     *
617
     * @param $request
618
     *
619
     * @return AjaxHTTPResponse|bool
620
     */
621
    public function clear($request)
622
    {
623
        $this->updateLocale($request);
624
        $this->cart->clear();
625
        $this->extend('updateClearResponse', $request, $response);
626
        return $response ? $response : self::direct();
627
    }
628
629
    /**
630
     * Handle index requests
631
     */
632
    public function index()
633
    {
634
        if ($cart = $this->Cart()) {
0 ignored issues
show
Documentation Bug introduced by
The method Cart does not exist on object<ShoppingCart_Controller>? Since you implemented __call, maybe consider adding a @method annotation.

If you implement __call and you know which methods are available, you can improve IDE auto-completion and static analysis by adding a @method annotation to the class.

This is often the case, when __call is implemented by a parent class and only the child class knows which methods exist:

class ParentClass {
    private $data = array();

    public function __call($method, array $args) {
        if (0 === strpos($method, 'get')) {
            return $this->data[strtolower(substr($method, 3))];
        }

        throw new \LogicException(sprintf('Unsupported method: %s', $method));
    }
}

/**
 * If this class knows which fields exist, you can specify the methods here:
 *
 * @method string getName()
 */
class SomeClass extends ParentClass { }
Loading history...
635
            $this->redirect($cart->CartLink);
636
            return;
637
        } elseif ($response = ErrorPage::response_for(404)) {
638
            return $response;
639
        }
640
        return $this->httpError(404, _t("ShoppingCart.NOCARTINITIALISED", "no cart initialised"));
641
    }
642
643
    /**
644
     * Displays order info and cart contents.
645
     */
646
    public function debug()
647
    {
648
        if (Director::isDev() || Permission::check("ADMIN")) {
649
            //TODO: allow specifying a particular id to debug
650
            Requirements::css(SHOP_DIR . "/css/cartdebug.css");
651
            $order = ShoppingCart::curr();
652
            $content = ($order)
653
                ?
654
                Debug::text($order)
0 ignored issues
show
Documentation introduced by
$order is of type object<Order>, but the function expects a object<unknown_type>.

It seems like the type of the argument is not accepted by the function/method which you are calling.

In some cases, in particular if PHP’s automatic type-juggling kicks in this might be fine. In other cases, however this might be a bug.

We suggest to add an explicit type cast like in the following example:

function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
Loading history...
655
                :
656
                "Cart has not been created yet. Add a product.";
657 3
            return array('Content' => $content);
0 ignored issues
show
Bug Best Practice introduced by
The return type of return array('Content' => $content); (array<string,unknown|string>) is incompatible with the return type of the parent method ViewableData::Debug of type ViewableData_Debugger.

If you return a value from a function or method, it should be a sub-type of the type that is given by the parent type f.e. an interface, or abstract method. This is more formally defined by the Lizkov substitution principle, and guarantees that classes that depend on the parent type can use any instance of a child type interchangably. This principle also belongs to the SOLID principles for object oriented design.

Let’s take a look at an example:

class Author {
    private $name;

    public function __construct($name) {
        $this->name = $name;
    }

    public function getName() {
        return $this->name;
    }
}

abstract class Post {
    public function getAuthor() {
        return 'Johannes';
    }
}

class BlogPost extends Post {
    public function getAuthor() {
        return new Author('Johannes');
    }
}

class ForumPost extends Post { /* ... */ }

function my_function(Post $post) {
    echo strtoupper($post->getAuthor());
}

Our function my_function expects a Post object, and outputs the author of the post. The base class Post returns a simple string and outputting a simple string will work just fine. However, the child class BlogPost which is a sub-type of Post instead decided to return an object, and is therefore violating the SOLID principles. If a BlogPost were passed to my_function, PHP would not complain, but ultimately fail when executing the strtoupper call in its body.

Loading history...
658
        }
659 3
    }
660 3
661
    protected function updateLocale($request)
662
    {
663 3
        $order = $this->cart->current();
664
        if ($request && $request->isAjax() && $order) {
665 2
            ShopTools::install_locale($order->Locale);
0 ignored issues
show
Documentation introduced by
The property Locale does not exist on object<Order>. Since you implemented __get, maybe consider adding a @property annotation.

Since your code implements the magic getter _get, this function will be called for any read access on an undefined variable. You can add the @property annotation to your class or interface to document the existence of this variable.

<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}

If the property has read access only, you can use the @property-read annotation instead.

Of course, you may also just have mistyped another name, in which case you should fix the error.

See also the PhpDoc documentation for @property.

Loading history...
666
        }
667
    }
668
}
669