@joomdonation It seems you need to update the phpstan-baseline file. This can be done after a composer install with command ./libraries/vendor/bin/phpstan -b.

@richard67 Not sure if it is on my Windows or not but running the command generates so many changes to the phpstan-baseline file. If you have time, could you please help me with this?

./libraries/vendor/bin/phpstan

@joomdonation Yes, lots of changes. But to me they look ok. There are lots of deprecations removed, which is the purpose of this PR.

All other changes are of the following type, replacing a deprecation message by another deprecation message. See one example:

Old

identifier: method.deprecated
count: 1
path: libraries/src/Event/User/LoginEvent.php
message: '''
#^Call to deprecated method setResult\(\) of class Joomla\\CMS\\Event\\User\\LogoutEvent\:
4\.4\.0 will be removed in 7\.0
Use counterpart with onSet prefix$#
'''

New

identifier: property.deprecated
count: 1
path: libraries/src/Event/User/LoginEvent.php
message: '''
#^Access to deprecated property \$preventSetArgumentResult of class Joomla\\CMS\\Event\\User\\LoginEvent\:4\.3 will be removed in 7\.0
Using setResult\(\) for the result argument will always be disallowed\.$#
'''

Note the difference in the identifier and the message.

There are several changes of the same kind.

To me it seems to make sense.

To you, too? Or does it need some more changes to get rid of these completely?

If it is ok as it is, I can push the changed phpstan-baseline.neon file to your branch for this PR.

@joomdonation I've created a PR in your repo with the phpstan-baseline.neon changes: joomdonation#7 @laoneo Could you check if that makes sense to you?

Thanks @richard67 . That looks much better than the file generated by the command in my computer. In my computer, there are lots of more changes. I attached the file on my computer for reference.
phpstan-baseline.zip

While code changes looks good it still need to test. There was a reason why I leave it ike that. Was some infinity loop or something, I do not remember the detail sanymore.

A lot will be tested with system tests. But some human tests would definitely be good. Wondering if we should rebase to 7.0 because of your infinite loop concern?

For testing, as Allon said, lots are covered by system tests. For manual testing, I personal tested create/edit/publish... article, same with user. I also tested Multi-factor Authentication, all works good

From code, these methods mostly used to pre-processing/validating event arguments data when event object is created. The method is called when it is needed, so don't see how infinity loop could happen here. To be sure, I wrote small script to test event constructor, call onSetXXX, setXXX method to see if infinity loop happens, but it won't.

$event = new \Joomla\CMS\Event\MultiFactor\BeforeDisplayMethods($this->app->getIdentity());

// Standard method call, usually happens in event contruction
$event->setArgument('user', new User(765));

// Manual call the deprecated method
$event->setUser(new User(765));

// Call this new method (for this method, I have to change the method to public to make it callable for testing)
$event->onSetUser(new User(765)); 


From my view, it is all good. And now you will have to do the hard work to make final decision :)

For testing, as Allon said, lots are covered by system tests. For manual testing, I personal tested create/edit/publish... article, same with user. I also tested Multi-factor Authentication, all works good

From code, these methods mostly used to pre-processing/validating event arguments data when event object is created. The method is called when it is needed, so don't see how infinity loop could happen here. To be sure, I wrote small script to test event constructor, call onSetXXX, setXXX method to see if infinity loop happens, but it won't.

$event = new \Joomla\CMS\Event\MultiFactor\BeforeDisplayMethods($this->app->getIdentity());

// Standard method call, usually happens in event contruction
$event->setArgument('user', new User(765));

// Manual call the deprecated method
$event->setUser(new User(765));

// Call this new method (for this method, I have to change the method to public to make it callable for testing)
$event->onSetUser(new User(765)); 

From my view, it is all good. And now you will have to do the hard work to make final decision :)

I guess the infinite loop was caused by extensions extending event classes and did set arguments in a different order. But can't reall remember as well maybe @wilsonge ?

That's why I think we should consider to rebase to 7.0.

I guess the infinite loop was caused by extensions extending event classes and did set arguments in a different order. But can't reall remember as well maybe @wilsonge ?

That does not happen. I wrote some code, create custom event class, create event object (both with arguments in right and wrong order, even tried to test it with arguments with legacy format) and it's still working OK

• Custom event class

class CustomEventClass extends \Joomla\CMS\Event\Table\BeforePublishEvent
{
	public function __construct($name, array $arguments = [])
	{
		parent::__construct($name, $arguments);

		$this->setState(0);
		$this->onSetState(1);
		$this->setUserId(100);
		$this->onSetState(200);
	}
}

• Test script to test create event objects with different cases

$db = Factory::getContainer()->get('db');
$row = new Content($db);

// Correct arguments order
$event = new BeforePublishEvent(
	'onTableBeforePublish',
	[
		'subject' => $row,
		'pks'     => [1, 2, 3],
		'state'   => 1,
		'userId'  => 765,
	]
);

// Random arguments order
$event = new BeforePublishEvent(
	'onTableBeforePublish',
	[
		'userId'  => 765,
		'subject' => $row,
		'state'   => 1,
		'pks'     => [1, 2, 3],
	]
);

// Custom event class with correct arguments order
$event = new CustomEventClass(
	'onTableBeforePublish',
	[
		'subject' => $row,
		'pks'     => [1, 2, 3],
		'state'   => 1,
		'userId'  => 765,
	]
);

// Custom event class with random arguments order
$event = new CustomEventClass(
	'onTableBeforePublish',
	[
		'userId'  => 765,
		'subject' => $row,
		'state'   => 1,
		'pks'     => [1, 2, 3],
	]
);

// Custom event class with arguments in legacy format
$event = new CustomEventClass(
	'onTableBeforePublish',
	[
		$row,
		[1, 2, 3],
		1,
		765,
	]
);

Could not think of any cases which cause infinite loop. But if we are still not confident, we can do it for 7.0

If I can recall correctly, then it was something with the override of the set or onset function.

Ah, Yes. If someone extends the class, and override onSet method, then call set method, it will cause infinite loop

I change the PR to 7.0-dev. But look like there are conflict between 6.2 and 7.0 branch. I convert the PR to draft for now and will continue when 6.2 is up-merged to 7.0. And we will need to decide if we should remove set*() methods on 7.0.

Upmerge is in progress #47997

Upmerge is in progress #47997

Great. Thanks

I think it is fine for 6.2,

I think it is fine for 6.2,
There was something when you call setFoobar, or something, then the event code doing call of setArgument which again doing call of setFoobar.
I think it was corrected in all the events by adding onSetFoobar.

@Fedik There is a case which I mentioned above. Not sure if it happens in real life but it will causes infinite loop as you and Allon worry about

If someone extends the class, and override onSet method, then call set method, it will cause infinite loop

Yes exactly, something like this prevented us in the past to make the switch. By the way, Upmerge is done.

Can you also adapt the target version in the depreciation message to 8.0?

@laoneo So you want to keep the set*() methods until J8? If so, in theory, we will still face the infinite loop problem

Sure, but on some point we need to do the switch. In a major will cause the least headache. Removeing the set functions in the same release when doing the switch would probably be very disruptive. We should give extension devs some time to migrate.

If someone extends the class, and override onSet method, then call set method, it will cause infinite loop

That would be a huge bug in their code. That should newer happen.
We can ignore this.

OK. I changed the messages to 8.0. These methods were marked as deprecated from 4.4.0, I think we can remove on 7.0, but that can be decided later.

set/get methods can be removed in 7.0,
I see no reason to wait to 8.0.

There are realted locgick in get/setArgument which should be removed in 7.0
All set/get which is marked as deprectaed for 7.0 can be removed in 7.0.

If someone extends the class, and override onSet method, then call set method, it will cause infinite loop
That would be a huge bug in their code. That should newer happen. We can ignore this.

Personal, I agree with this. But I just want to stay on safe side because both of you worried about that. If maintainers accept this on 6.2, I will change it back

onSet and onGet was added to avoid confusion and avoid such looping, the situation you guys talking is imaginary.
Just ignore that 😉

onSet and onGet was added to avoid confusion and avoid such looping, the situation you guys talking is imaginary. Just ignore that 😉

Who knows how creative developers could do :). But I will put this to maintenance meeting and let maintainers decide