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

Remove from cart throws an exception #101

Closed
junkystu opened this Issue Aug 24, 2015 · 16 comments

Comments

Projects
None yet
5 participants
@junkystu

junkystu commented Aug 24, 2015

The remove() function returns null rather than boolean true when removing items. After passing it a non existing rowid, it throws an Gloudemans\Shoppingcart\Exceptions\ShoppingcartInvalidRowIDException exception with empty message rather than return false.

@it-can

This comment has been minimized.

Show comment
Hide comment
@it-can

it-can Aug 31, 2015

I am also experiencing this error in Laravel 5.1.12

it-can commented Aug 31, 2015

I am also experiencing this error in Laravel 5.1.12

@lbodden

This comment has been minimized.

Show comment
Hide comment
@lbodden

lbodden Sep 24, 2015

can confirm not working in laravel 5.1.16

lbodden commented Sep 24, 2015

can confirm not working in laravel 5.1.16

@Crinsane

This comment has been minimized.

Show comment
Hide comment
@Crinsane

Crinsane Sep 24, 2015

Owner

Please supply me the code that doesn't work. Because I've just tested it and everything is ok.

New Laravel installation version 5.1.17:

Add a new item to the cart:

Route::get('/', function () {
    Cart::add('293ad', 'Product 1', 1, 9.99, ['size' => 'large']);
});

When I next dump the content of the cart dd(Cart::content()); I get one item in the cart with rowId: 0f6524cc3c576d484150599b3682251c

Next remove the item from the cart:

Route::get('delete', function () {
    Cart::remove('0f6524cc3c576d484150599b3682251c');
});

Item is removed, no exception thrown...

Owner

Crinsane commented Sep 24, 2015

Please supply me the code that doesn't work. Because I've just tested it and everything is ok.

New Laravel installation version 5.1.17:

Add a new item to the cart:

Route::get('/', function () {
    Cart::add('293ad', 'Product 1', 1, 9.99, ['size' => 'large']);
});

When I next dump the content of the cart dd(Cart::content()); I get one item in the cart with rowId: 0f6524cc3c576d484150599b3682251c

Next remove the item from the cart:

Route::get('delete', function () {
    Cart::remove('0f6524cc3c576d484150599b3682251c');
});

Item is removed, no exception thrown...

@junkystu

This comment has been minimized.

Show comment
Hide comment
@junkystu

junkystu Sep 26, 2015

@Crinsane I've used pretty much the same code as you with the same version of Laravel, new install. When I dd the content, I get the item with its rowId.

Route::get('/', function () {
    Cart::add('293ad', 'Product 1', 1, 9.99, ['size' => 'large']);
    dd(Cart::content());
});

Route::get('delete', function () {
    // dd(Cart::get('0f6524cc3c576d484150599b3682251c'));
    Cart::remove('0f6524cc3c576d484150599b3682251c');
});

On the delete page I get the exception ShoppingcartInvalidRowIDException in Cart.php line 191.

If I dump Cart::get('0f6524cc3c576d484150599b3682251c'), I get back null.
With that in mind, it looks like the item is somehow not found but either way, I think the delete function should not throw an exception if the item is not found given that get() does not throw an exception.
testing local-delete

Update: It looks like dd(Cart::content()) after adding the item is responsible for the exception. If you add and the dd(Cart::content()) or dd(Cart::get('0f6524cc3c576d484150599b3682251c')) on base url and then go to /delete, you will see an exception. However, adding the item to cart without getting the item by ID or the entire content and then going to /delete will not throw the exception.

junkystu commented Sep 26, 2015

@Crinsane I've used pretty much the same code as you with the same version of Laravel, new install. When I dd the content, I get the item with its rowId.

Route::get('/', function () {
    Cart::add('293ad', 'Product 1', 1, 9.99, ['size' => 'large']);
    dd(Cart::content());
});

Route::get('delete', function () {
    // dd(Cart::get('0f6524cc3c576d484150599b3682251c'));
    Cart::remove('0f6524cc3c576d484150599b3682251c');
});

On the delete page I get the exception ShoppingcartInvalidRowIDException in Cart.php line 191.

If I dump Cart::get('0f6524cc3c576d484150599b3682251c'), I get back null.
With that in mind, it looks like the item is somehow not found but either way, I think the delete function should not throw an exception if the item is not found given that get() does not throw an exception.
testing local-delete

Update: It looks like dd(Cart::content()) after adding the item is responsible for the exception. If you add and the dd(Cart::content()) or dd(Cart::get('0f6524cc3c576d484150599b3682251c')) on base url and then go to /delete, you will see an exception. However, adding the item to cart without getting the item by ID or the entire content and then going to /delete will not throw the exception.

@Crinsane

This comment has been minimized.

Show comment
Hide comment
@Crinsane

Crinsane Sep 26, 2015

Owner

Try to first do the Cart::add() and no dd(). Then comment the Cart::add() and uncomment the dd()
Now you should see content, and a rowId. Now you should be able to delete it.

When you perform a dd() method, it will make the script die instantly, so the session is not written.

Owner

Crinsane commented Sep 26, 2015

Try to first do the Cart::add() and no dd(). Then comment the Cart::add() and uncomment the dd()
Now you should see content, and a rowId. Now you should be able to delete it.

When you perform a dd() method, it will make the script die instantly, so the session is not written.

@junkystu

This comment has been minimized.

Show comment
Hide comment
@junkystu

junkystu Sep 26, 2015

@Crinsane , yes, that seems to be what's happening. However, if you refresh the delete page, you will still see an exception because you are trying to remove and ID that no longer exists in the cart content. The issue is that an exception is thrown and it's not documented. The documentation says:

/**
 * Remove a row from the cart
 *
 * @param  string  $rowId The rowid of the item
 * @return boolean
 */

But remove() always returns null upon successfully removing an item or throws an exception if the item is not found.

junkystu commented Sep 26, 2015

@Crinsane , yes, that seems to be what's happening. However, if you refresh the delete page, you will still see an exception because you are trying to remove and ID that no longer exists in the cart content. The issue is that an exception is thrown and it's not documented. The documentation says:

/**
 * Remove a row from the cart
 *
 * @param  string  $rowId The rowid of the item
 * @return boolean
 */

But remove() always returns null upon successfully removing an item or throws an exception if the item is not found.

@Crinsane

This comment has been minimized.

Show comment
Hide comment
@Crinsane

Crinsane Sep 26, 2015

Owner

Aaah I see. Could you send a pull request with the updated docblocks?

Owner

Crinsane commented Sep 26, 2015

Aaah I see. Could you send a pull request with the updated docblocks?

@junkystu

This comment has been minimized.

Show comment
Hide comment
@junkystu

junkystu Sep 26, 2015

I would rather update it so that it returns boolean. Please let me know if you'd like me to do that.

junkystu commented Sep 26, 2015

I would rather update it so that it returns boolean. Please let me know if you'd like me to do that.

@Crinsane

This comment has been minimized.

Show comment
Hide comment
@Crinsane

Crinsane Sep 26, 2015

Owner

You can give it a shot

Owner

Crinsane commented Sep 26, 2015

You can give it a shot

@junkystu

This comment has been minimized.

Show comment
Hide comment
@junkystu

junkystu Sep 26, 2015

I gave it a go! I've noticed some inconsistencies in other docblocks so I'll update those too when I get the chance.

junkystu commented Sep 26, 2015

I gave it a go! I've noticed some inconsistencies in other docblocks so I'll update those too when I get the chance.

@Harrrrry

This comment has been minimized.

Show comment
Hide comment
@Harrrrry

Harrrrry Oct 26, 2015

I feel the same problem,
ShoppingcartInvalidRowIDException in Cart.php line 191.

my action
public function deleteshortlist() {
$rowId = Cart::search(array('id' => Request::get('product_id')));
Cart::remove($rowId[0]);
return view('myshortlist', array('cart' => $cart, 'title' => 'Welcome', 'description' => '', 'page' => 'home'));
}

view link href
href="{{ url('/welcome/'.$fdw->rowid.'/deleteshortlist/')}}"

and routes
Route::get('/welcome/{id}/deleteshortlist', 'WelcomeController@deleteshortlist');

Can any one suggest me a solution

Harrrrry commented Oct 26, 2015

I feel the same problem,
ShoppingcartInvalidRowIDException in Cart.php line 191.

my action
public function deleteshortlist() {
$rowId = Cart::search(array('id' => Request::get('product_id')));
Cart::remove($rowId[0]);
return view('myshortlist', array('cart' => $cart, 'title' => 'Welcome', 'description' => '', 'page' => 'home'));
}

view link href
href="{{ url('/welcome/'.$fdw->rowid.'/deleteshortlist/')}}"

and routes
Route::get('/welcome/{id}/deleteshortlist', 'WelcomeController@deleteshortlist');

Can any one suggest me a solution

@Crinsane

This comment has been minimized.

Show comment
Hide comment
@Crinsane

Crinsane Oct 26, 2015

Owner

@Harrrrry What problem? What code? How can I reproduce this?
Can't give a suggestion if you don't give us anything to work with.

Owner

Crinsane commented Oct 26, 2015

@Harrrrry What problem? What code? How can I reproduce this?
Can't give a suggestion if you don't give us anything to work with.

@Harrrrry

This comment has been minimized.

Show comment
Hide comment
@Harrrrry

Harrrrry Oct 26, 2015

@Crinsane I updated my question with complete code, please take a look.

Harrrrry commented Oct 26, 2015

@Crinsane I updated my question with complete code, please take a look.

@junkystu

This comment has been minimized.

Show comment
Hide comment
@junkystu

junkystu commented Oct 26, 2015

@Crinsane @Harrrrry my pull request has fixed this issue.

@Harrrrry

This comment has been minimized.

Show comment
Hide comment
@Harrrrry

Harrrrry Oct 26, 2015

@Crinsane @junkystu thanks, i will check.

Harrrrry commented Oct 26, 2015

@Crinsane @junkystu thanks, i will check.

@junkystu

This comment has been minimized.

Show comment
Hide comment
@junkystu

junkystu Oct 27, 2015

@Crinsane Why wasn't the pull request merged in? Does anything require changing?

junkystu commented Oct 27, 2015

@Crinsane Why wasn't the pull request merged in? Does anything require changing?

@Crinsane Crinsane closed this Nov 21, 2015

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