[#39469] - [5.3] Make abstract immutable event actually immutable
- Pending
- Medium
- Build: 5.0-dev
- # 39469
- Diff
- wilsonge:abstract_immutable_event_not_immutable
User tests: Successful: Unsuccessful:
Pull Request for Issue #39372 .
Summary of Changes
As pointed out in the issue our immutable event is very mutable. This fixes all the cases so that once initialised there can be no properties set into the event (it’s possible we might need some plug-in events to actually be mutable).
As we've now split the Immutable class from the mutable class - I've taken this opportunity to move the create method to it's own factory class - because it will be somewhat confusing for users that the mutable AbstractEvent class can create AbstractImmutable class's which are somewhat unrelated.
Testing Instructions
Ensure core continues to work when performing a variety of actions. We also should test this with some 3rd party plugins to ensure they also still work.
Actual result BEFORE applying this Pull Request
Event is mutable
Expected result AFTER applying this Pull Request
Event is not mutable
Link to documentations
Please select:
-
Documentation link for docs.joomla.org:
-
No documentation changes for docs.joomla.org needed
-
Pull Request link for manual.joomla.org:
-
No documentation changes for manual.joomla.org needed
joomla-cms-bot
- | Category | ⇒ | Libraries |
| Status | New | ⇒ | Pending |
| Title |
|
||||||
@SharkyKZ Why confused? Because of what the PR does? Or because it's for 5.0?
Because it doesn't make much sense for this immutable class to extend a class that is explicitly mutable. Any change here is going to be a B/C break anyways so might as well fix the inheritance tree.
| Labels |
Added:
PR-5.0-dev
|
||
| Category | Libraries | ⇒ | Administration com_content Modules Libraries Front End Plugins |
Because it doesn't make much sense for this immutable class to extend a class that is explicitly mutable. Any change here is going to be a B/C break anyways so might as well fix the inheritance tree.
That's a fair shout. The framework class is final so couldn't extend from there but implemented as similar now
If this PR is accepted I'll do a PR to migrate the EventFactory creation back to the 4.x CMS tree
I'm not sure how useful it is to introduce a new class for a functionality which we should not use.
Every event should have it's own class and should be created directly without a factory. ymmv
Whilst the class exists it should do what it says on the tin :) yvmv :P
Is this PR now absolute after the all new events?
This pull request has been automatically rebased to 5.1-dev.
In order to make the AbstractEventImmutable class true immutable, you disabled the ability to call setArgument after initialization. It's the right way! But what about methods addArgument, removeArgument, clearArguments?
| Labels |
Added:
Feature
b/c break
|
||
| Labels |
Added:
PR-5.1-dev
|
||
This pull request has been automatically rebased to 5.2-dev.
| Title |
|
||||||
This pull request has been automatically rebased to 5.3-dev.
| Title |
|
||||||
@SharkyKZ Why confused? Because of what the PR does? Or because it's for 5.0?