Skip to content

Comments

[6.1] Make abstract immutable event actually immutable#39469

Open
wilsonge wants to merge 7 commits intojoomla:6.1-devfrom
wilsonge:abstract_immutable_event_not_immutable
Open

[6.1] Make abstract immutable event actually immutable#39469
wilsonge wants to merge 7 commits intojoomla:6.1-devfrom
wilsonge:abstract_immutable_event_not_immutable

Conversation

@wilsonge
Copy link
Contributor

@wilsonge wilsonge commented Dec 22, 2022

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

@wilsonge wilsonge requested a review from HLeithner December 22, 2022 07:52
@wilsonge wilsonge changed the title Make abstract immutable event actually immutable [5.0] Make abstract immutable event actually immutable Dec 22, 2022
@richard67
Copy link
Member

@SharkyKZ Why confused? Because of what the PR does? Or because it's for 5.0?

@SharkyKZ
Copy link
Contributor

@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.

@wilsonge wilsonge requested a review from bembelimen as a code owner December 29, 2022 20:59
@wilsonge
Copy link
Contributor Author

wilsonge commented Dec 29, 2022

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

@wilsonge
Copy link
Contributor Author

wilsonge commented Jan 3, 2023

@SharkyKZ can you re-review please

@HLeithner
Copy link
Member

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

@HLeithner HLeithner added Feature b/c break This item changes the behavior in an incompatible why. HEADS UP labels Apr 7, 2023
@wilsonge
Copy link
Contributor Author

Whilst the class exists it should do what it says on the tin :) yvmv :P

@HLeithner
Copy link
Member

Is this PR now absolute after the all new events?

@HLeithner HLeithner changed the base branch from 5.0-dev to 5.1-dev September 30, 2023 22:50
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.1-dev.

@kernusr
Copy link
Contributor

kernusr commented Mar 13, 2024

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?

@wilsonge
Copy link
Contributor Author

@kernusr we're extending the frameworks' abstractEvent here now - so these methods don't exist at all

@HLeithner HLeithner changed the base branch from 5.1-dev to 5.2-dev April 24, 2024 09:08
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.2-dev.

@HLeithner HLeithner changed the title [5.0] Make abstract immutable event actually immutable [5.2] Make abstract immutable event actually immutable Apr 24, 2024
@HLeithner HLeithner changed the base branch from 5.2-dev to 5.3-dev September 2, 2024 08:53
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.3-dev.

@HLeithner HLeithner changed the title [5.2] Make abstract immutable event actually immutable [5.3] Make abstract immutable event actually immutable Sep 2, 2024
@Hackwar Hackwar removed the PR-5.2-dev label Sep 3, 2024
@HLeithner HLeithner changed the base branch from 5.3-dev to 6.0-dev March 4, 2025 17:22
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 6.0-dev.

@HLeithner HLeithner changed the title [5.3] Make abstract immutable event actually immutable [6.0] Make abstract immutable event actually immutable Mar 4, 2025
@rdeutz rdeutz removed the PR-5.3-dev label Mar 5, 2025
@exlemor exlemor moved this to Abandoned in PR Cleanup May 10, 2025
@HLeithner HLeithner changed the base branch from 6.0-dev to 6.1-dev August 31, 2025 12:00
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 6.1-dev.

@HLeithner HLeithner changed the title [6.0] Make abstract immutable event actually immutable [6.1] Make abstract immutable event actually immutable Aug 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

b/c break This item changes the behavior in an incompatible why. HEADS UP Feature PR-6.0-dev PR-6.1-dev

Projects

Status: Abandoned

Development

Successfully merging this pull request may close these issues.

9 participants