Storing stuff in $SESSION not ideal

moodle-an-hochschulen / moodle-local_bulkenrol

Moodle plugin which provides the possibility to bulk enrol a list of users who are identified by their e-mail adresses into a course.

5 stars 17 forks source link

Storing stuff in $SESSION not ideal #6

Closed danmarsden closed 6 years ago

danmarsden commented 6 years ago

Typically it's considered bad practice to store plugin related stuff in the global $SESSION var - you might consider finding a different way to store this data: https://github.com/moodleuulm/moodle-local_bulkenrol/blob/master/index.php#L72

I don't think this will block approval in the plugins db but it's probably worth adjusting at some point.

mudrd8mz commented 6 years ago

I don't personally see this as a big issue.

The plugin is correctly using the SESSION wrapper and not the PHP's native $_SESSION superglobal.
It stores data there in its own namespace.

As far as I understand it, the preferred way these days would be a properly defined MUC session cache maybe but the current code does not look necessarily wrong to me.

danmarsden commented 6 years ago

Yeah - not a big issue for a 3rd party plugin. Feel free to close as won't fix! :-)

abias commented 6 years ago

Hi Dan,

I finally found the time to polish this plugin for plugin repo approval. Thanks for this hint.

While I would generally agree and think that this should better be a MUC session cache, this is the way the original developer built the plugin and I am happy that this is no show-stopper for approval.

Closing as won't fix ;)

Cheers, Alex