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

a non numeric value encountered category pagination 7.1 #7218

Closed
ravilrrr opened this issue Feb 18, 2019 · 15 comments

Comments

Projects
None yet
5 participants
@ravilrrr
Copy link

commented Feb 18, 2019

a non numeric value encountered in category.php if page=

$page = $this->request->get['page'];

replace

$page = (int)$this->request->get['page'];

@ravilrrr

This comment has been minimized.

Copy link
Author

commented Feb 18, 2019

and manufacturer and etc

@straightlight

This comment has been minimized.

Copy link

commented Feb 18, 2019

One point I may need to address here is that the $page variables being passed into the common/pagination.php file are not an issue but only when it's not being passed into that controller becomes an issue since the $page variable is not sanitized and may need to be …

Another option would to be pass the $page variable as a reference (&$page) into the common/pagination.php file and ensure that the variable is sanitized before exiting the constructor. This would avoid on having to manually sanitize each of these variables from each controllers that demands it.

As for the manufacturer, I am not quite sure what you are referring to specifically.

@ravilrrr

This comment has been minimized.

Copy link
Author

commented Feb 18, 2019

see
'start' => ($page - 1) * $limit,
https://github.com/opencart/opencart/blob/master/upload/catalog/controller/product/category.php#L158
https://github.com/opencart/opencart/blob/master/upload/catalog/controller/product/manufacturer.php#L148

if page as string (&page={user deleted it himself}) you see Warning
a non numeric value encountered

@ravilrrr

This comment has been minimized.

@straightlight

This comment has been minimized.

Copy link

commented Feb 18, 2019

I see what you mean. However, setting a straight (int) at the front of the requested page number could lead to 0 while the default page value is 1.

Perhaps, this should help:

if (isset($this->request->get['page']) && (int)$this->request->get['page'] > 0) { $page = (int)$this->request->get['page']; } else { $page = 1; }

@ravilrrr

This comment has been minimized.

Copy link
Author

commented Feb 18, 2019

You are right.

@IP-CAM

This comment has been minimized.

Copy link

commented Feb 21, 2019

works also in v.1.5.6.4+5 Versions, as shown by straightlight

@schiggi

This comment has been minimized.

Copy link

commented Mar 4, 2019

Yes, this problem also exists at least on OC 2.3, probably on lower versions as well.

Suggestion would be to use !empty, i.e.
if (!empty($this->request->get['page'])) { $page = (int)$this->request->get['page']; } else { $page = 1; }

That should catch non-existing as well as 0 values

@straightlight

This comment has been minimized.

Copy link

commented Mar 4, 2019

Your code above suggests to check for an empty integer but as well for other variable types which the page query only requires an integer compared to my above code where it checks if the page key is set and that the converted page key to an integer has a page number higher than 0; otherwise - the page default value is 1.

@schiggi

This comment has been minimized.

Copy link

commented Mar 4, 2019

Fair enough. Negative values would be a problem in my suggestion.

danielkerr added a commit that referenced this issue Mar 5, 2019

@danielkerr

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

solved it by

	if (isset($this->request->get['page'])) {
		$page = (int)$this->request->get['page'];
	} else {
		$page = 1;
	}

@danielkerr danielkerr closed this Mar 5, 2019

@straightlight

This comment has been minimized.

Copy link

commented Mar 5, 2019

By using the code above, the 0 (zero) value won't be discarded as the default page=1 won't appear on the browser since page 0 is invalid. A book contains at least 1 page. A book can't be considered a book if there are no pages to show inside it.

@straightlight

This comment has been minimized.

Copy link

commented Mar 9, 2019

The only downside of using my suggestion is that it would involve page redirection. I believe why the simple isset validation without sanitizing the value in the IF condition has been set is to avoid page redirection on a page product where search engines already tracks its media.

In this case, rather than regrouping the products per page by the browser with forced redirection whenever an invalid page number is involved; than to address the error/not_found page instead. This would also provide an understanding to the bots that these pages would indeed be invalid without forcing redirection.

@straightlight

This comment has been minimized.

Copy link

commented Mar 9, 2019

In the catalog/controller/common/pagination.php file, it would seem my suggestion already exists:

if (isset($setting['page']) && $setting['page'] > 0) {

It could simply, however, be replaced with:

if (isset($setting['page']) && (int)$setting['page'] > 0) {

This would solved all the problems with the Github version.

@straightlight

This comment has been minimized.

Copy link

commented Mar 10, 2019

I have provided a better and definite solution on the OC forum: https://forum.opencart.com/viewtopic.php?f=24&t=210459#p748892 . This solution also converts all page=0 queries back to 1 despite of the URL length.

Resolution confirmation: https://forum.opencart.com/viewtopic.php?f=24&t=210459&p=748912#p748907

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.