Crinsane / LaravelShoppingcart

A simple shopping cart implementation for Laravel
MIT License
3.66k stars 1.72k forks source link

Store method not working correctly with PostgreSQL driver #362

Open fearrr opened 7 years ago

fearrr commented 7 years ago

The basket store method does not work, when using PostgreSQL driver. The collection is not fully preserved, and during use of restore method, we got error.

Crinsane commented 7 years ago

What is not working? What is the error? I need more information to help you.

fearrr commented 7 years ago

An error occurs when you try to call the restore method (But in fact the store method works incorrectly)

unserialize(): Error at offset 42 of 45 bytes
at HandleExceptions->handleError(8, 'unserialize(): Error at offset 42 of 45 bytes', 'PATH\\mvm\\vendor\\gloudemans\\shoppingcart\\src\\Cart.php', 381, array('identifier' => 1, 'stored' => object(stdClass)))

content field value in shoppingcart table, it turned out badly cropped

O:29:"Illuminate\Support\Collection":1:{s:8:"

my CreateShoppingcartTable migration run method

Schema::create('shoppingcart', function (Blueprint $table) {
            $table->string('identifier');
            $table->string('instance');
            $table->text('content');
            $table->nullableTimestamps();

            $table->primary(['identifier', 'instance']);
        });
Crinsane commented 7 years ago

Which version of Laravel are you using? And which version of the package? Is there any way for me to reproduce this problem?

fearrr commented 7 years ago

laravel 5.4.*, postgresql 9.6, package ^2.3.

Crinsane commented 7 years ago

can you verify for me that the type of the content column is actually text (or similar) and not something like varchar(45) or something? Because I can't reproduce this error.

fearrr commented 7 years ago

This is the first thing I checked. screenshot

Crinsane commented 7 years ago

I was pretty sure you had 😉 But I'm running out of options. Can't really seem to be able to reproduce this.

fearrr commented 7 years ago

The problem in php code, because I tried on different operating systems (windows, linux) & different versions of postgresql. What can I do to help find the problem?

fearrr commented 7 years ago

set breakpoint screenshot_10

get serialized collection :29:"Illuminate\Support\Collection":1:{s:8:"\x00*\x00items";a:7{s:32:"b3fef508225c0bc74d12efbb30e22f3d";O:32:"Gloudemans\Shoppingcart\CartItem":8:...and more

in database writing this sting :29:"Illuminate\Support\Collection":1:{s:8:"

screenshot_11

The same string but before the \x00 An error occurs while writing a \x00 symbol

Crinsane commented 7 years ago

I've been doing some reading, and seems to be a problem with Postgres PDO and null bytes in the serialized string. PHP uses those null bytes to store private/protected information for the methods of a class it is serializing.

Could you please try if anything changes if you change the $associatedModel and $taxRate properties on the CartItem class to be public.

fearrr commented 7 years ago

I set public on $associatedModel and $taxRate properties, but it did not work. I also tried to insert serialize collection into DB manually, result is the same...

Crinsane commented 7 years ago

Any more \x00 in the serialized string when you make those properties public?

fearrr commented 7 years ago

yes, screenshot_14

Crinsane commented 7 years ago

It seems to be something Postgres specific. Maybe making the content column in the table another datatype (so not text) will fix the problem, but I'm not that familiar with Postgres to tell you which datatype to use. Maybe Google could help you with this

fearrr commented 7 years ago

I could not find any other information. Because postgres now allows store arrays and json in its original form. As I understand for text there is only text type.

I would suggest you use mutator and store the collection in the database in json or array... At least for the postgre driver

fearrr commented 7 years ago

Postgres is particularly relevant for e-commerce, since it avoids the slow model EAV

sbourdette commented 7 years ago

I have the same thigns with a mongodb : "\u0000

It would be great that Cart class extend eloquent to store directly and not serialize / unserialize

sbourdette commented 7 years ago

Exemple :

Cart content : {"da130ea67758fbaeb7d260ebb707e746":{"rowId":"da130ea67758fbaeb7d260ebb707e746","id":"59810abf0574071a102c4065","name":"A trier","qty":"154","price":0,"options":{"type":"album","albumPicture":"59810abf0574071a102c4066"},"tax":0,"subtotal":0}}

Cart in the mongodb : { "_id" : ObjectId("5988bfdd05740705e212c3d2"), "identifier" : "59810abb0574071a0b1b1502", "instance" : "default", "content" : "O:29:\"Illuminate\\Support\\Collection\":1:{s:8:\"\u0000*\u0000items\";a:1:{s:32:\"da130ea67758fbaeb7d260ebb707e746\";O:32:\"Gloudemans\\Shoppingcart\\CartItem\":8:{s:5:\"rowId\";s:32:\"da130ea67758fbaeb7d260ebb707e746\";s:2:\"id\";s:24:\"59810abf0574071a102c4065\";s:3:\"qty\";s:3:\"154\";s:4:\"name\";s:7:\"A trier\";s:5:\"price\";d:0;s:7:\"options\";O:39:\"Gloudemans\\Shoppingcart\\CartItemOptions\":1:{s:8:\"\u0000*\u0000items\";a:2:{s:4:\"type\";s:5:\"album\";s:12:\"albumPicture\";s:24:\"59810abf0574071a102c4066\";}}s:49:\"\u0000Gloudemans\\Shoppingcart\\CartItem\u0000associatedModel\";N;s:41:\"\u0000Gloudemans\\Shoppingcart\\CartItem\u0000taxRate\";i:21;}}}" }

Error message when trying to restore :

` [2017-08-07 19:38:58] local.ERROR: ErrorException: Trying to get property of non-object in /home/vagrant/Code/openpicture/vendor/gloudemans/shoppingcart/src/Cart.php:381 Stack trace:

0 /home/vagrant/Code/openpicture/vendor/gloudemans/shoppingcart/src/Cart.php(381): Illuminate\Foundation\Bootstrap\HandleExceptions->handleError(8, 'Trying to get p...', '/home/vagrant/C...', 381, Array)

1 /home/vagrant/Code/openpicture/vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php(221): Gloudemans\Shoppingcart\Cart->restore('59810abb0574071...')

`

Crinsane commented 7 years ago

Yes, seems to be certain database/datatypes will have problems with those escaped strings. For now I'd try to Google for another datatype that will be able to store serialized data from php with those nullbytes

sbourdette commented 7 years ago

to make it work i had to modify restore function in Cart.php :

` $storedContent = unserialize($stored['content']); $currentInstance = $this->currentInstance(); $this->instance($stored['instance']);

`

I don't understand why i have to access like an array. Maybe there is some automatic cast

fearrr commented 7 years ago

why reinvent the wheel? Laravel has built-in mutator for store collections and arrays in database

yoosuf commented 6 years ago

any updates on this issue?

fearrr commented 6 years ago

question to owner

tuananhpham2008 commented 6 years ago

?????????

yoosuf commented 6 years ago

The solution what I did was, created an line_items table and dumped those records on it and started using it, seems working fine as expected, I think it will be the best interim solution.

anx-mwutte commented 6 years ago

Hi, the bug is caused by Null-bytes, due the fact, that Postgres does not support them. The problem is caused by the "Cart" class in src/Cart.php.

An quick and dirty fix would be to replace the Null-bytes before storing the serialized string into database (http://php.net/manual/de/function.serialize.php#96504).

    /**
     * Store an the current instance of the cart.
     *
     * @param mixed $identifier
     * @return void
     */
    public function store($identifier)
    {
        $content = $this->getContent();

        if ($this->storedCartWithIdentifierExists($identifier)) {
            throw new CartAlreadyStoredException("A cart with identifier {$identifier} was already stored.");
        }

        $this->getConnection()->table($this->getTableName())->insert([
            'identifier' => $identifier,
            'instance' => $this->currentInstance(),
            'content' => str_replace("\0", "~~NULL_BYTE~~", serialize($content))
        ]);

        $this->events->fire('cart.stored');
    }

    /**
     * Restore the cart with the given identifier.
     *
     * @param mixed $identifier
     * @return void
     */
    public function restore($identifier)
    {
        if( ! $this->storedCartWithIdentifierExists($identifier)) {
            return;
        }

        $stored = $this->getConnection()->table($this->getTableName())
            ->where('identifier', $identifier)->first();

        $storedContent = unserialize(str_replace("~~NULL_BYTE~~", "\0", $stored->content));

        $currentInstance = $this->currentInstance();

        $this->instance($stored->instance);

        $content = $this->getContent();

        foreach ($storedContent as $cartItem) {
            $content->put($cartItem->rowId, $cartItem);
        }

        $this->events->fire('cart.restored');

        $this->session->put($this->instance, $content);

        $this->instance($currentInstance);

        $this->getConnection()->table($this->getTableName())
            ->where('identifier', $identifier)->delete();
    }

However it would be better, to find a more standardized solution, because the serialize-function is not very safe, and so malicious content could be injected.

ChristianKreuzberger commented 6 years ago

Same problem here! Postgres does not properly support the PHP serialized "Strings" (frankly speaking, \x00 defines the end of a string in any good programming language).

Wouldn't it be possible to just use a JSON Serialization instead? Or to customize the serialize method?

miladkian commented 6 years ago

@Crinsane any updates on this issue?

zimberkazmiz commented 6 years ago

problem was solved using base64_encode after serializing and base64_decode before unserializing content. see my fork at https://github.com/zimberkazmiz/LaravelShoppingcart/blob/master/src/Cart.php