SeasideSt / Seaside

The framework for developing sophisticated web applications in Smalltalk.
MIT License
519 stars 71 forks source link

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

Closed JoachimTuchel closed 1 year ago

JoachimTuchel commented 1 year ago

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) gets added to newer versions of Seaside.

jbrichau commented 1 year ago

duplicate of #1353