BackwardCompatibility Refactor

[tyrsson]

Removes BackwardCompatibility Trait function exporting
Provides function mapping via Subs-Compat.php
Improves typing for methods that explicitly have phpdoc comments stating what typing should be.

[jdarwood007]

Looking good!

As mentioned to you in chat. Since you fixed some of the type handling here. Take note I have done work in both #7923 and #7927 (only fixes phpdoc) which are not merged yet which also fix types. I haven't touched anything inside actions yet as the core code needs fixed before we handle it into actions.
For those wanting to know more why, when the core is fixed, actions are either using the core code correctly, or we need to fix the core to handle the types and update the phpdoc.

[tyrsson]

Well, I was thinking on this. If you work those changes and I work the actions then we shouldn't have conflicts and we get better coverage. That however is up to @Sesquipedalian and whether we decide to go this route. Its still kinda up in the air at the moment. I just opened this Draft so he could take a look around.

[Sesquipedalian]

I like where this is going.

My first suggestion is that the right place for all these wrapper functions is the Subs-Compat.php file. Things like this are the reason for that's file's existence.

My second suggestion is that it is premature to kill of the BackwardCompatibilty trait just yet. We need an alternative way to deal with exporting static properties to global variables before we can do that.

So for now, I suggest putting the trait back the way it was and moving all the wrapper functions to Subs-Compat.php. All the places where you have stripped out references to the trait so far can stay that way, and as you work through more of them you can continue in the same vein.

You'll need to leave the trait in place in any classes that have a prop_names subarray in their $backcompat property until we come up with a good alternative for dealing with the properties.

Also, don't forget to remove all the calls to the exportStatic() at the end of each class's file. It looks like some have been missed in this draft.

[tyrsson]

The export Static calls were just missed in the process of just getting the forum to load so I could continue :) I'm aware that they need cleaned up :). Will pull off the move to Subs-Compat here shortly and cleanup those files as well as reinstate the Trait.

[Sesquipedalian]

Most comments are just reminders to remove the $backcompat property, but there are a few more substantial questions and comments in there.

[Sesquipedalian]

Forgot to remove the $backcompat property and the call to exportToStatic() at the end of the file.

[tyrsson]

Look at the Sources/Actions/Admin/* classes. I have not worked on hardly any of these top level classes other than to get the forum to load. As I commit I note which the commit is finishing in the commit message. Those are the ones ready for review. Will save us both a lot of time :)

[Sesquipedalian]

Please restore the missing blank line.

Can also remove the 'func_names' subarray from the $backcompat property below.

[tyrsson]

I only targeted one function/method pair from this file for testing very early on. The single function has been defined as first in the list will move when I get to that class.

[tyrsson]

@Sesquipedalian I considered the suggestions on the composed trait approach, but thought better of it since this is basically just a temp shim. I will implement those sometime over the weekend.

Also, agree on the other suggestions.

Hope to have it completed by maybe Tuesday of this coming week.

[tyrsson]

@Sesquipedalian I think I covered your requested changes.

I maintained all of the factory/provider methods although I moved them to their respective classes so they are not available via composition to classes that can not and should not have access to them. In that context I think they make sense since it allows us to remove many no longer needed methods and provides a consistent pattern to back compat usage throughout the application.

I introduced the two new traits and implemented those. One usage of particular note is that Actions\TrackIP uses the Profile\BackwardCompatibility Trait instead of the Actions\ trait. Reason for this is that the Profile traits subActionProvider signature already had support for the $memID argument and I did not want to introduce a signature change for a single method and argument.

I moved the discussed methods from Utils to Subs-Compat and removed them from Utils. Once this is approved I will then remove the related deprecated methods that the traits replace.

[dragomano]

To make backward compatibility work in the case of permissions, we need to modify the code of the integrateLoadPermissions method in the Sources/Actions/Admin/Permissions.php:

find

// Provide a practical way to modify permissions.
IntegrationHook::call('integrate_load_permissions', [&self::$permission_groups, &$permissions_by_scope, &self::$left_permission_groups, &$hidden_permissions, &$relabel_permissions]);

replace with

// Provide a practical way to modify permissions.
IntegrationHook::call('integrate_load_permissions', [&self::$permission_groups, &$permissions_by_scope, &self::$left_permission_groups, &$hidden_permissions, &$relabel_permissions]);

if (isset(self::$permission_groups['membergroup'])) {
	self::$permission_groups['global'] = array_merge(self::$permission_groups['global'], self::$permission_groups['membergroup']);
	unset(self::$permission_groups['membergroup']);
}

if (isset($permissions_by_scope['membergroup'])) {
	$permissions_by_scope['global'] += $permissions_by_scope['membergroup'];
	unset($permissions_by_scope['membergroup']);
}

[tyrsson]

To make backward compatibility work in the case of permissions, we need to modify the code of the integrateLoadPermissions method in the Sources/Actions/Admin/Permissions.php:

find

// Provide a practical way to modify permissions.
IntegrationHook::call('integrate_load_permissions', [&self::$permission_groups, &$permissions_by_scope, &self::$left_permission_groups, &$hidden_permissions, &$relabel_permissions]);

replace with

// Provide a practical way to modify permissions.
IntegrationHook::call('integrate_load_permissions', [&self::$permission_groups, &$permissions_by_scope, &self::$left_permission_groups, &$hidden_permissions, &$relabel_permissions]);

if (isset(self::$permission_groups['membergroup'])) {
	self::$permission_groups['global'] = array_merge(self::$permission_groups['global'], self::$permission_groups['membergroup']);
	unset(self::$permission_groups['membergroup']);
}

if (isset($permissions_by_scope['membergroup'])) {
	$permissions_by_scope['global'] += $permissions_by_scope['membergroup'];
	unset($permissions_by_scope['membergroup']);
}

@Sesquipedalian, @jdarwood007 what's your thoughts on this?

[Sesquipedalian]

@Sesquipedalian, @jdarwood007 what's your thoughts on this?

On the one hand, @Bugo usually knows what he is talking about. On the other hand, we don't have an explanation of why these suggested changes are necessary.

So, @Bugo, please open an issue to explain what is wrong with this hook and why we need to change it.

Regarding this PR, it doesn't change anything about that integration hook, so I don't think we need to worry about it here.

[jdarwood007]

@tyrsson You have a conflict

Otherwise, I think we should merge and work out the issue with the permissions when we have more details. I'm not certain what the problem is at the moment.

[Sesquipedalian]

1. In my previous review, I made this comment:

   There are a number of cases where classes have static methods that exist solely for the purpose of facilitating backward compatibility. For most of these, it would make sense to move the entire contents of the method into the relevant Subs-Compat function.

   This was meant to include to the various backCompatProvider methods that are called from some of the functions in Subs-Compat.php.

   For example:

   function AutoSuggestHandler(?string $suggest_type = null): ?bool
   {
   	return Actions\AutoSuggest::backCompatProvider(suggest_type: $suggest_type, callHandler: true);
   }
   

   should simply be:

   function AutoSuggestHandler(?string $suggest_type = null): ?bool
   {
   	if (isset($suggest_type)) {
   		return Actions\AutoSuggest::checkRegistered($suggest_type);
   	}
   
   	Actions\AutoSuggest::call();
   }
   

2. You forgot to delete the existing backward compatibility static methods from many of the classes that no longer need them.

3. Don't forget to run PHP-CS-Fixer. It flagged a bunch of little things when I checked out this PR most recently. 🙂

[tyrsson]

I'll work on this first thing in the morning.

[dragomano]

On the one hand, @Bugo usually knows what he is talking about. On the other hand, we don't have an explanation of why these suggested changes are necessary.

So, @Bugo, please open an issue to explain what is wrong with this hook and why we need to change it.

Regarding this PR, it doesn't change anything about that integration hook, so I don't think we need to worry about it here.

Currently, SMF 2.1 has no problem with this hook. So, I can't create an issue now. But when this PR will be merged, the problem will appear. In particular, I've been tested adding permissions with Light Portal.

Before PR changes:

After PR changes:

First of all, we see a missing title on the permissions block:

And we have some new errors in the Error Log:

In SMF 3.0, for some reason, the self::$permission_groups parameter in this hook stops having a membergroup key and uses the global key. My code just adds a little tweak to get around this :)

[tyrsson]

@Sesquipedalian @jdarwood007 @dragomano

I think it covered everything. Please let me know if I missed anything.

[dragomano]

I think, randomBytes and randomInt should be static.

[Sesquipedalian]

I think, randomBytes and randomInt should be static.

Yes, they should.

[Sesquipedalian]

There were a few changes to make, but it was simpler to just make them than to document them.

Thanks for this, @tyrsson. Excellent work!