Read-only event variables

General discussion of development ideas and the approaches taken in the 3.x branch of phpBB. The current feature release of phpBB 3 is 3.3/Proteus.
Forum rules
Please do not post support questions regarding installing, updating, or upgrading phpBB 3.3.x. If you need support for phpBB 3.3.x please visit the 3.3.x Support Forum on phpbb.com.

If you have questions regarding writing extensions please post in Extension Writers Discussion to receive proper guidance from our staff and community.
Post Reply
CHItA
Development Team
Development Team
Posts: 169
Joined: Thu Mar 12, 2015 1:43 pm
Location: Budapest, Hungary

Read-only event variables

Post by CHItA »

There is a problem with our current way of handling events in the core. I think it would be a good idea to restrict certain variables from being modified in certain events. The main problem with the current system is that it event variables can be changed in events where it makes no sense which causes the number of possible execution paths to explode. This makes it really hard to refactor old code without the possibility of breaking BC (there are countless examples of this in viewtopic and viewforum). E.g. $forum_data is in every single event fired from viewforum.

I understand that extension authors might want to read data in events, however, from an architectural stand point it cannot really be guaranteed that a certain piece of information will be present where the event will be fired after refactoring the code. Marking these event variables read-only would allow to eliminate a large number of execution paths, which would allow us to be able to produce better migration guides in case an event has to be changed. This is currently impossible to do as no human can possibly understand how our legacy code works.

I'd be happy to all opinions regarding this matter as IMHO we need to come up with a solution to this problem which allow both the dev team and ext. authors to have a better workflow and more flexibility in the future.

Edit: To be clear this would only apply to certain event variables, you would still be able to manipulate some of them. The purpose of read-only variables to make it clear what can be modified from an event.

User avatar
Marc
Development Team Leader
Development Team Leader
Posts: 185
Joined: Thu Sep 09, 2010 11:36 am
Location: Munich, Germany

Re: Read-only event variables

Post by Marc »

I think we've done this before in some places by having a copy of certain variables that is then later unset and not used. I think it's a good idea to have this possibility and to be able to use it where needed.
The bigger question to me is how to implement this functionality to support an easy and clean declaration. Do you already have ideas on how to accomplish this?

User avatar
Elsensee
Former Team Member
Posts: 42
Joined: Sun Mar 16, 2014 1:08 pm
Location: Hamburg, Germany
Contact:

Re: Read-only event variables

Post by Elsensee »

Like Marc said, in some places we do already implement an behaviour like that, see e.g.:
https://github.com/phpbb/phpbb/blob/11d ... r.php#L283
(In that case we just don't copy that variable because we won't use it anyway after that)

I think there should be some cleaner way to do this which makes the intention already clear by just reading the code. Unfortunately, with our current way of testing event descriptions this is not yet possible.

I can think of a way to make either all variables read-only or none of them but I guess we also want to have the possibility of some mixed cases?

User avatar
mrgoldy
Former Team Member
Posts: 64
Joined: Fri Dec 18, 2015 9:41 pm
Location: The Netherlands
Contact:

Re: Read-only event variables

Post by mrgoldy »

Could you provide an example that would create a lot of possible execution paths?
And isn't the way of thinking supposed to be: The core code works and if an extension breaks it using an event, the extension is at fault.
It should either handle the 'alternative execution path' itself or not alter it in the first place.
phpBB Studio Proud member of the Studio!

CHItA
Development Team
Development Team
Posts: 169
Joined: Thu Mar 12, 2015 1:43 pm
Location: Budapest, Hungary

Re: Read-only event variables

Post by CHItA »

Marc wrote: Sun Sep 08, 2019 11:01 am The bigger question to me is how to implement this functionality to support an easy and clean declaration. Do you already have ideas on how to accomplish this?
Well, first of I think we should just start documenting it in some way. Then we can start to think about if actually restricting the modification should be enforced by the implementation or just make it a requirement on the extension validation process etc.
Elsensee wrote: Sun Sep 08, 2019 1:43 pm I can think of a way to make either all variables read-only or none of them but I guess we also want to have the possibility of some mixed cases?
Yes, IMO in most cases it should be mixed.
posey wrote: Mon Sep 09, 2019 3:34 pm Could you provide an example that would create a lot of possible execution paths?
And isn't the way of thinking supposed to be: The core code works and if an extension breaks it using an event, the extension is at fault.
It should either handle the 'alternative execution path' itself or not alter it in the first place.
Well, the problem comes in when it is not specified what an extension can modify something and where they may modify it. Since this is not documented in a consistent way, it is really up to personal taste as of where it makes sense to someone.

If you want examples just look at viewtopic and viewforum, practically forum_id, topic_id, forum_data and topic_data is in all events.

User avatar
mrgoldy
Former Team Member
Posts: 64
Joined: Fri Dec 18, 2015 9:41 pm
Location: The Netherlands
Contact:

Re: Read-only event variables

Post by mrgoldy »

I understand the identifiers. They should probably be static.
However, for example forum_data or topic_data is sometimes altered by extensions.
I know of extensions (among others some of my/our own) that add (add, not alter) data to an event variable that is available within multiple events in a file. To pass on data from one event to another. For example to save on SQL queries or making use of variables that are not available in the second event.

(this is by heart, so might be slightly off, but it's just an example)
For example our Dice Rolls extensions;
When retrieving the post identifiers in a viewtopic page, we grab all dice rolls at once.
Then when iterating over the posts and displaying their data, we can grab the corresponding dice rolls.

A solution would be to add an empty $data variable and pass it along multiple events, so extensions can use/fill that instead.
phpBB Studio Proud member of the Studio!

CHItA
Development Team
Development Team
Posts: 169
Joined: Thu Mar 12, 2015 1:43 pm
Location: Budapest, Hungary

Re: Read-only event variables

Post by CHItA »

posey wrote: Mon Sep 09, 2019 6:30 pm I understand the identifiers. They should probably be static.
However, for example forum_data or topic_data is sometimes altered by extensions.
I know of extensions (among others some of my/our own) that add (add, not alter) data to an event variable that is available within multiple events in a file. To pass on data from one event to another. For example to save on SQL queries or making use of variables that are not available in the second event.
That is fine, the problem is that depending on what you modify practically anything may happen in our legacy front controllers. The problem is not really that some extensions might be broken by changes, it is rather that it is hard to understand the possible use cases without actually looking at all extensions using a specific event (which is impossible to do). So what this topic is about is coming up with a solution for the time when we moved all that code to controllers and e.g. need to refactor again. By that time it should be clear from the core code that what changes an event allows the ext authors to do. Currently we only have an event name which should indicate what the event is for, however, this does not seem to be sufficient. Better defined events would also allow us on the dev side to have better support/docs for BC breaks, e.g. if it is obvious what an event may do, and we refactor it in a BC breaking way, we could document what events to use instead to keep the extension functioning.

Post Reply