View Issue Details

This bug affects 1 person(s).
 12
IDProjectCategoryView StatusLast Update
15380Bug reportsPluginspublic2020-01-17 14:14
Reporterbismark Assigned Tocdorin  
PrioritynoneSeverityminor 
Status closedResolutionno change required 
Summary15380: Create class or Interface for PluginEvent with names as constants
Description

this helps for IDE-Code Completion

e.g.


interface PluginEventInterface
{
    const EVENT_BEFORE_CONTROLLER_ACTION = 'beforeControllerAction';
}

class PluginEvent implements PluginEventInterface
{
}

//LSYII_Controller::beforeControllerAction
$event = new PluginEvent(PluginEvent::EVENT_BEFORE_CONTROLLER_ACTION);
$event->set('controller', $controller->getId());
$event->set('action', $action->getId());
$event->set('subaction', Yii::app()->request->getParam('sa'));
App()->getPluginManager()->dispatchEvent($event);
TagsNo tags attached.
Bug heat12
Complete LimeSurvey version number (& build)develop
I will donate to the project if issue is resolvedNo
Browser
Database type & version10.1.26-MariaDB
Server OS (if known)
Webserver software & version (if known)
PHP Version7.1.8

Users monitoring this issue

DenisChenu

Activities

bismark

bismark

2019-10-07 16:58

reporter   ~53930

more benefits:

  • event names centralized
  • better documentation
ollehar

ollehar

2019-10-08 14:32

administrator   ~53948

can an interface include constants?

DenisChenu

DenisChenu

2019-10-08 15:28

developer   ~53953

My opinion :

  1. event name must be unique in all LimeSurvey code
  2. event name is used one time only in Plugin

Sometimes i'm unsure add CONSTANT make more readable code : https://github.com/LimeSurvey/LimeSurvey/blob/6cba3e0c964df2cc8430e103b0fa6baec3e97480/application/helpers/questionHelper.php#L71

Auto documentation with such solution can not include input and output
Then no real reason for this.

dschirge

dschirge

2019-10-09 14:15

reporter   ~53973

I agree with Bismark, it would be very useful for future development.
I would implement it this way:

use MabeEnum\Enum;
/**

  • @method Events BEFORE_CONTROLLER_ACTION()
    **/
    class Events extends Enum
    {
    // Enumerators will be generated from public constants only
    public const BEFORE_CONTROLLER_ACTION = 'beforeControllerAction';
    }

additionally benefits:

  • you can compare event types internal if necessary and even constants with the same value are different because of the Enum implementation.

  • type safety

  • the already mentioned benefits

Example:
public const EV_A = 'a';
puclic const EV_B = 'a';

Events:: EV_A()->is(Events::EV_B()); //false
It is because if the is() implementation which does a comparisment of the value and the name of the enumerator.

Mazi

Mazi

2019-10-09 15:03

updater   ~53974

@c_schmitz, can you comment on this? For us - and other plugin developers - it would be pretty useful and I am rather sure it will not have any side effects when adding this to the core.

dschirge

dschirge

2019-10-11 22:26

reporter   ~54013

I created an example and uploaded to a fork in GitHub.
You can loot at this in https://github.com/pop1989bb/LimeSurvey/commit/b516890335d66f0ec295ba7f6cc4614b621a9b9e
You coud use the new system via $this->subscribe(GlobalEvents::AFTER_FIND_SURVEY()->getValue());
The function "getValue()" returns the string I assigned to the enum.
I suggest to use it this way for compatibility reasons. In a future version (maybe v4 or v5) I advise to enforce using the enums. It will reduce errors.
I didn't implement all events nor made comments for all of the already declared events. I would do this if my suggestion is accepted.

dschirge

dschirge

2019-11-12 20:26

reporter   ~54550

as wished I added a smaller commit.
https://github.com/pop1989bb/LimeSurvey/commit/a0437365dd07747c860aeea3d0bd3db871d2d460

DenisChenu

DenisChenu

2019-11-13 10:32

developer   ~54551

Last edited: 2019-11-13 10:32

  1. Can you add a start part about each event params (readable and writable) documentation ?
  2. This class must allow some way to add event : sample : beforeTokenEmailExtended or tripleSfieldMap
DenisChenu

DenisChenu

2019-11-13 10:35

developer   ~54552

Last edited: 2019-11-13 10:35

Did we really need the _ in constant name ? EVENTNAMEINCAMELCASE = 'evantNameInCamelCase'?

dschirge

dschirge

2019-11-13 10:46

reporter   ~54553

1) I will do an example if this is what you mean.
2) I'm unsure what do you mean. Do you want an example how it could look like with the class?

the question: Yes, we need them. It is defined in PSR-1 section 1. Look at https://www.php-fig.org/psr/psr-1/
"Class constants MUST be declared in all upper case with underscore separators."
PSR means "PHP Standard Recommendation."

DenisChenu

DenisChenu

2019-11-13 10:50

developer   ~54555

Not with mandatoty underscore (or ?), we are on https://www.php-fig.org/psr/psr-2/

About 2 : i don't know … if i know i don't need to ask … but if you have a Class to manage event : this class must offer same service to plugins.

I know it's currently just an Enum and nothing else, but : evolution …

dschirge

dschirge

2019-11-13 10:54

reporter   ~54556

We are on https://www.php-fig.org/psr/psr-1/ as I quotedt. :) "Class constants MUST be declared in all upper case with underscore separators.", the underscores in constants are mandatory.
About 2). I wouldn't recommend this. An enum should be an enum and not more. You shouldn't add other functions to it in my view.
If we add further functions we hurt the SOLID principle.

DenisChenu

DenisChenu

2019-11-13 10:57

developer   ~54557

Const NOUNDERSCORE is valid … underscore is not mandatory …

About 2 : then you say plugin can not create event … in think it's a mistake

dschirge

dschirge

2019-11-13 11:07

reporter   ~54558

PSR-2 is regarded as deprecated and replaced with PSR-12. But anyway it's a recommendation. I personally prefery underscore for better reading. I can read EVENT_NAME_IN_CAMEL_CASE better than EVENTNAMEINCAMELCASE.

2) I never mentioned that a plugin can't dispatch an event. Of course it can. The SOLID principle describes how classes should be designed. The S means "Single responsitivity". The class "GlobalEvents" only should provide the constants for events.
But I now understand what you mean. You want to dispatch events which aren't defined in for example "GlobalEvents". Of course you can design the system for allowing custom events. It's just a question of architecture.

DenisChenu

DenisChenu

2019-11-13 11:15

developer   ~54559

About underscore : yes EVENT_NAME_IN_CAMELCASE but our event name are evantNameInCamelCase (no underscore) , there here… just unsure :)

Of course you can design the system for allowing custom events. It's just a question of architecture.

Yes, i think we need to mind of possible evolution at start of development, even if we don't add it concretely (no code, just some doc on How we can do in future, possible evolution).

dschirge

dschirge

2019-11-13 11:27

reporter   ~54561

Ah ok. :)
Regarding the naming. In my already pretty long career as PHP developer I noticed that it's common sense to write constants in upper case. Of course it's no must but a help. At least for me. ;-)

Events:
Just an idea:

<code php>class LimeSurveyEvents extends mabe/Enum</code>

<code php>class GlobalEvents extens LimeSurveyEvents</code>

Event dispatcher allows LimeSurveyEvents

Your plugin has an own class SondagesProEvents
<code php>class SondagesProEvents extends LimeSurveyEvents
{
public constant BEFORE_TOKEN_EMAIL_EXTENDED='BEFORE_TOKEN_EMAIL_EXTENDED';
}
</code>

And now the dispatcher would accept custom events.
Of course it's jsuit a first idea without knowing the LS eventing system. But maybe it's a hint for a direction.

dschirge

dschirge

2019-11-13 11:32

reporter   ~54562

Ah ok. :)
Regarding the naming. In my already pretty long career as PHP developer I noticed that it's common sense to write constants in upper case. Of course it's no must but a help. At least for me. ;-)

Events:
Just an idea:

class LimeSurveyEvents extends mabe/Enum
class GlobalEvents extens LimeSurveyEvents

Event dispatcher allows LimeSurveyEvents

Your plugin has an own class SondagesProEvents

class SondagesProEvents extends LimeSurveyEvents
{
public constant BEFORE_TOKEN_EMAIL_EXTENDED='BEFORE_TOKEN_EMAIL_EXTENDED';
}

And now the dispatcher would accept custom events.
Of course it's just a first idea without knowing the LS eventing system. But maybe it's a hint for a direction.

PS: Sorry for the douple post. I'm just learning the system.

dschirge

dschirge

2019-12-19 05:57

reporter   ~55070

@DenisChenu
What about my thoughts? :)

DenisChenu

DenisChenu

2019-12-19 08:10

developer   ~55071

Didn't really know :) sorry …

dschirge

dschirge

2019-12-19 12:15

reporter   ~55075

OK. :) Let's see if there is any development in this direction.

DenisChenu

DenisChenu

2019-12-19 12:19

developer   ~55076

@dschirge : remind : i am not in LimeSurveyy internal team, then : https://manual.limesurvey.org/How_to_contribute_new_features

@cdorin ? @c_schmitz ? @ollehar ? for discussion.

dschirge

dschirge

2019-12-19 12:24

reporter   ~55077

@DenisChenu: Sorry, didn't know. I am still new to LS.

Mazi

Mazi

2020-01-14 15:25

updater   ~55234

@dschirge: At today's developer meeting it was concluded that the gain of the suggested improvement is minimal so it was decided to close this one for now.
Thanks for all your efforts!

Mazi

Mazi

2020-01-14 15:25

updater   ~55235

@cdorin, please close this one.

Issue History

Date Modified Username Field Change
2019-10-07 16:07 bismark New Issue
2019-10-07 16:58 bismark Note Added: 53930
2019-10-08 14:32 ollehar Note Added: 53948
2019-10-08 15:28 DenisChenu Note Added: 53953
2019-10-09 14:15 dschirge Note Added: 53973
2019-10-09 15:03 Mazi Note Added: 53974
2019-10-11 22:26 dschirge Note Added: 54013
2019-11-12 14:55 DenisChenu Issue Monitored: DenisChenu
2019-11-12 20:26 dschirge Note Added: 54550
2019-11-13 10:25 DenisChenu Note View State: 54013: private
2019-11-13 10:25 DenisChenu Note View State: 54013: public
2019-11-13 10:32 DenisChenu Note Added: 54551
2019-11-13 10:32 DenisChenu Note Edited: 54551
2019-11-13 10:35 DenisChenu Note Added: 54552
2019-11-13 10:35 DenisChenu Note Edited: 54552
2019-11-13 10:46 dschirge Note Added: 54553
2019-11-13 10:50 DenisChenu Note Added: 54555
2019-11-13 10:54 dschirge Note Added: 54556
2019-11-13 10:57 DenisChenu Note Added: 54557
2019-11-13 11:07 dschirge Note Added: 54558
2019-11-13 11:15 DenisChenu Note Added: 54559
2019-11-13 11:27 dschirge Note Added: 54561
2019-11-13 11:32 dschirge Note Added: 54562
2019-12-19 05:57 dschirge Note Added: 55070
2019-12-19 08:10 DenisChenu Note Added: 55071
2019-12-19 12:15 dschirge Note Added: 55075
2019-12-19 12:19 DenisChenu Note Added: 55076
2019-12-19 12:24 dschirge Note Added: 55077
2020-01-14 15:25 Mazi Note Added: 55234
2020-01-14 15:25 Mazi Note Added: 55235
2020-01-17 14:14 cdorin Assigned To => cdorin
2020-01-17 14:14 cdorin Status new => closed
2020-01-17 14:14 cdorin Resolution open => no change required
2021-08-03 10:27 guest Bug heat 10 => 12