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

yiiGridView pagination stops working on ajax request #4570

Open
Skalin opened this issue Oct 26, 2024 · 13 comments
Open

yiiGridView pagination stops working on ajax request #4570

Skalin opened this issue Oct 26, 2024 · 13 comments

Comments

@Skalin
Copy link

Skalin commented Oct 26, 2024

The pagination and or filtration of grid views stops working upon trying to filter using Ajax request. This is caused by an updated jquery-bbq library. The library fails in "qs.HasOwnProperty" method.

What steps will reproduce the problem?

Trying to refresh, paginate or filter the Yii gridview causes javascript to fail. This is done even before any AJAX request is done.
Simple call for: $("#grid-view").yiiGridView("update", {url: refreshURL}); will cause the bug.

What is the expected result?

The grid view is properly updated and the ajax request is processed.

What do you get instead?

Error response:

index:461 Uncaught TypeError: qs.hasOwnProperty is not a function
	at index:461:12
	at Function.<anonymous> (jquery.min.js:4:17819)
	at Function.each (jquery.min.js:2:2881)
	at g (jquery.min.js:4:17785)
	at Ub (jquery.min.js:4:17922)
	at Function.ajax (jquery.min.js:4:21073)
	at HTMLDivElement.<anonymous> (jquery.yiigridview.js:355:21)
	at Function.each (jquery.min.js:2:2881)
	at n.fn.init.each (jquery.min.js:2:846)
	at n.fn.init.update (jquery.yiigridview.js:260:16)

More info - the issue occurs in the function ajax.preFilter:

$.ajaxPrefilter(function (options, originalOptions, jqXHR) {
	var qs = $.deparam.querystring(options.url);
	if (qs.hasOwnProperty("ajax") && qs.ajax == "grid-view")

Additional info

Q A
Yii version 1.1.30
PHP version 8.2
Operating system Debian
jQuery version v1.12.4 (Yii packed one)
@Skalin
Copy link
Author

Skalin commented Oct 26, 2024

The temporary solution is to revert back to package 1.1.29.
There might be a possibility to update jquery to newer versino using client script, but it has to be tested which version will work properly.

@marcovtwout
Copy link
Member

marcovtwout commented Oct 28, 2024

@kevin-foster-uk It seems #4563 has caused a regression, could you take a look at this?

@Skalin Could you point out exactly where you think the update jquery-bbq has caused this? I don't see it in your stack trace.

@Skalin
Copy link
Author

Skalin commented Oct 28, 2024

Well the javascripts throws the error on the: "qs.hasOwnProperty" method call inside the ajaxPreFilter method.

I believe that the bbq library expects the deparam function to return object. I tried logging it from console and it returns the object, but somehow it does not have the methods of the object prototype.

Theoretically, rewriting the call "qs.hasOwnProperty("ajax")" to "Object.prototype.hasOwnProperty.call(qs, 'ajax')" should work. But I am not sure whether the issue would occur anywhere else.

@kevin-foster-uk
Copy link
Contributor

@Skalin Can I ask what browser you are using? This update removed support for old IE browsers (edge is still supported).

In my testing using chrome, the ajax pagination is working fine.

Animation

@Busterkinng88
Copy link

🤴

@Skalin
Copy link
Author

Skalin commented Nov 4, 2024

Sorry for a bit of false alarm guys. :)

The issue happens in the Bootstrap extension (https://www.yiiframework.com/extension/bootstrap https://github.com/clevertech/YiiBooster), it expects that the querystring returns proper object, but the bbq returns some weird kind of object which does not have the "hasOwnProperty" method. To be precise, the issue occurs here: https://github.com/clevertech/YiiBooster/blob/c1d377a82581250558e7a6ff446ef7088f859f9e/src/widgets/TbExtendedGridView.php#L756

This can be fixed by updating the bootstrap extension, nevertheless I think that the proper fix should be that the deparam.querystring from bbq library returns a standard Object.

What do you think?

@marcovtwout
Copy link
Member

@Skalin The docblock above the function states it should return an object: https://github.com/LimeSurvey/yii/blob/ae2af5c65bb12dbd99cf0d69fdbaa4cc0b5f0ee1/framework/web/js/source/jquery.ba-bbq.js#L472

Can you post a full stack trace from your browsers inspector with clear input and output from the $.deparam function? What return path is taken inside the function?

@kevin-foster-uk
Copy link
Contributor

The doc block is not correct. It will return undefined if the param is not found (line #569). The changes we made recently result in undefined being returned if the parameter is prohibited (line #494).

I am not sure what the solution is here. I guess we could return an empty string instead of null.

@Skalin
Copy link
Author

Skalin commented Nov 11, 2024

Hi,

I am sending the whole trace that I got.

Input and ouptut of deparam.querystring method:

xpage:511 $.deparam.querystring(/en/xadmin/xpage/page/index?Page[id]=&Page[name]=&Page[title]=&Page[layout]=&Page[active]=1&Page_page=1&ajax=page-grid&yw3_options=on) =  
{
   Page: 
   {
         active: "1"
         id: ""
         layout: ""
         name: ""
         title: ""
   }
   Page_page: "1"
   ajax: "page-grid"
   yw3_options: "on"
}

Calling then the following method result in an exception:

 qs.hasOwnProperty("ajax") =

Error trace of the method (I am sorry, Yii has already the minified version and I was not able to make it use the non-minified version):

(anonymní) @ xpage:511
(anonymní) @ jquery.min.js:4
each @ jquery.min.js:2
g @ jquery.min.js:4
Ub @ jquery.min.js:4
ajax @ jquery.min.js:4
(anonymní) @ jquery.yiigridview.js:355
each @ jquery.min.js:2
each @ jquery.min.js:2
update @ jquery.yiigridview.js:260
$.fn.yiiGridView @ jquery.yiigridview.js:418
(anonymní) @ jquery.yiigridview.js:142
dispatch @ jquery.min.js:3
r.handle @ jquery.min.js:3
handleMouseUp_ @ neznámý
xpage:516 Uncaught TypeError: qs.hasOwnProperty is not a function
    at xpage:516:10
    at Function.<anonymous> (jquery.min.js:4:17819)
    at Function.each (jquery.min.js:2:2881)
    at g (jquery.min.js:4:17785)
    at Ub (jquery.min.js:4:17922)
    at Function.ajax (jquery.min.js:4:21073)
    at HTMLDivElement.<anonymous> (jquery.yiigridview.js:355:21)
    at Function.each (jquery.min.js:2:2881)
    at n.fn.init.each (jquery.min.js:2:846)
    at n.fn.init.update (jquery.yiigridview.js:260:16)

I think that this all comes to jquery.ba-bbq.js, where the object is being created with "null" as param:


    var obj = Object.create(null),
      coerce_types = { 'true': !0, 'false': !1, 'null': null };

Creating object this way results in missing methods like hasOwnProperty.
Creating object as this will result in methods like "hasOwnProperty" to be present:

Object.create({});

Is this enough information?

@marcovtwout
Copy link
Member

Reading the diagnosis and code again, the method still always returns object in the final line, consistent with the docblock. But Stalin is correct, what changed is that object no longer contains prototype methods:

@kevin-foster-uk When you create an object with Object.create(null), it will not inherit any object methods from prototype (see https://stackoverflow.com/questions/15518328/is-creating-js-object-with-object-createnull-the-same-as). Could you explain why you changed from var obj = {} to var obj = Object.create(null), was this intentional?

@kevin-foster-uk
Copy link
Contributor

kevin-foster-uk commented Nov 11, 2024

@marcovtwout The Object.create(null) is from the original PR submitted to the upstream (by @cee-chen) that we based our fix on. Thinking about it now, I can see this change is unnecessary since we now have an explicit check for prohibitedKeys (proto).

I will create a new PR to fix the issue.

@marcovtwout
Copy link
Member

marcovtwout commented Nov 11, 2024

@kevin-foster-uk I would suggest to first push this upstream cowboy/jquery-bbq#62 / cowboy/jquery-bbq#61, that way more eyes can look at the security fix.

@Skalin Regardless of this fix, I suggest to submit a PR to https://github.com/clevertech/YiiBooster as well. Looking at that code, it doesn't really need "hasOwnProperty" but could just do qs.ajax !== undefined as well. A similar issue was reported earlier as well: clevertech/YiiBooster#1052

@kevin-foster-uk
Copy link
Contributor

A PR has been created: #4571

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants