JQPlugin>>arguments should possibly send self options instead of accessing the inst variable

[JoachimTuchel]

I think I found a bug in JQPlugin long ago and just stumbled over it when I migrated my code from VAST 10.0.2 to 12.0.0. I once changed a method in my image because of a problem and I think this should be changed in Seaside base code as well.

JQPlugin>>arguments looks like this in Pharo 9, 10 and 11 (and VAST):

arguments
    ^ options isNil
        ifTrue: [ Array new ]
        ifFalse: [ Array with: options ]

But I think it should be

arguments
    ^ self options isNil
        ifTrue: [ Array new ]
        ifFalse: [ Array with: options ]

Why?

Because 'options`is lazily initialized:

options
    ^ options ifNil: [ options := GRSmallDictionary2 new ]

So if you subclass JQPlugin and implement something like

arguments
    |arg|
     arg := super arguments.

You get an empty Array instead of an Array with a GRSmallDictionary2 in its first slot. So if later in arguments you try to add to arg first you get an error because there is nothing at: 1 . And you cannot add anything to arg, because Arrays are fixed size and you can't add to it.

Most of the subclasses of JQPlugin shipping with Seaside implement their own logic for arguments and create some other collection, so you only realize this if you implement your own subclasses and send super arguments.

That's why I consider this a bug, even if it only shows when you subclass JQPlugin. If you agree, I'd be grateful if this little change (send self options instead of accessing options) to newer versions of Seaside.

[jbrichau]

The proposed change makes sense to me.

[jbrichau]

On second thought (and after investigating the code a little more): It would not make sense to have self options isNil since self options would never be nil. The method explicitly checks if options was never initialized and then does not pass it on as an argument to the function call. Triggering the (lazy) initialization of options in the arguments method would be the same as always initializing it.

I think the JQPlugin>>arguments method would better be made abstract with the current implementation pushed down to JQAjaxSetup. I would thus recommend not doing a super call in this hierarchy. Of course, I'm happy to take a second look if I can understand the example of doing so better.

[JoachimTuchel]

Hmm. Most subclasses of JQPlugin return an OrderedCollection anyways, so you may very well be right. In my subclasses, I am using Dictionaries in order to send out HashTables to my plugins. Maybe I made wrong assumptions about the fact that arguments accesses options and options typically creates a GrSmallDictionary, so I just stuffed my settings into arg first at: ... put: ...

The fact that arguments returns a Collection with optionsas first entry (if there are options), made me believe I need to augment the options, but that is not correct, obviously.

There is nothing keeping me from removing my super sends and creating my favorite Dictionary, however. Not sure I understand the reasoning behind arguments' way of operation at all if there is no use in reusing options.... unless you are saying the only subclass of JQPlugin (apart from mine, which are not your problem ;-) ) that really uses and needs the options in arguments is JQAjaxSetup. I was under the impression that was intended as default behavior and it just happens that the JQPlugin subclasses shipping with Seaside all don't need default behavior. That would not be the first time for me to be thinking way too compliacted ;-)

So I'll go and change (all) my subclasses to create their own Dictionary as arguments and remove the extension to Seaside in my code, and we're fine. That way my next migration to a newer Seaside version will be easier.

Thanks for looking into this.