Fixed configuration name typo

[Judx]

Description (*)

This PR fixes a typo in a configuration name in the system XML. The incorrect configuration name crate_admin_user_notification has been corrected to create_admin_user_notification.

Related Pull Requests

#3722

Fixed Issues (if relevant)

No.

Manual testing scenarios (*)

1. Navigate to the admin configuration: System -> Admin -> Security -> New Admin User Create Notification
2. Check the updated configuration name is create_admin_user_notification.
3. Test saving the configuration to ensure it works with the corrected name.
4. Optionally make sure email is sent to general store email when creating a new admin user and option "New Admin User Create Notification" is enabled.

Questions or comments

As part of this change, I would like to discuss on how to handle the transfer of the existing configuration values when users upgrade to the new version. The goal is to migrate the values from the old configuration name (crate_admin_user_notification) to the new one (create_admin_user_notification) seamlessly during the upgrade process.
We could do core_setup script that copies the value but that would need that we upgrade the module version that has not been done before. Any insights or recommended approaches for this would be greatly appreciated.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[addison74]

Thank you for your PR.

It is a harmless typo in my opinion and maybe it should be left like that. In a new installation the modification is fine, but in an existing one if this PR is adopted there must be two values in the configuration and not just one. This is because if any other extension uses that initial value, by deleting it using un upgrade script of the database, it will no longer work. I don't know how many extensions or custom code use this configuration value.

If this PR is merged then a script to upgrade the record into the database must be implemented.

[Judx]

If this PR is merged then a script to upgrade the record into the database must be implemented.

Yes. Standard and automatic (no user commands required) way of updating values like this in M1 is to:

1. Bump the module version
2. Add core_setup script that copies the old value to the new field that runs automatically when new module version is detected.

However this does not solve the problem of other modules depending on the old value name but I think that is normal part of version upgrades. Regarding this specific case I hardly believe any module is extending the "New Admin User Create Notification" true/false value.

[Judx]

I bumped the version added the setup script. How does it look?
36b58f5

I have another PR related to this I will be soon pushing.

[addison74]

I don't have any custom modification that depends on that value, but it is only one opinion so far.

I didn't test the update script but I looked over the code and it should make the change.

[Judx]

Nice! My tests successfully upgraded the path in db.

Here is the feature request I mentioned, it currently implements changes from this PR: #3722

[sreichel]

Mmmm, it will break extions that use that config path.

[fballiano]

IMHO we should implement this change (probably nobody implemented something based on this config path) but in the next branch.

[addison74]

I would approve as it is. I doubt that there is any extension that uses that path. It doesn't matter if it is main or next branch, if someone uses a production database, there will be both values available.

[fballiano]

oki, I'm willing to approve it as it is on the main branch, although I think we should be a bit stricter with the breaking-changes and technically this one is. If we'd be a bit stricter it would be actually more correct and we'd have a bigger changelog for the v21 which would also make it more clear that we need to release it ;-)

[fballiano]

Suggested change

	$table = $installer->getTable('core/config_data');
	if ($installer->getConnection()->isTableExists($table)) {
	$oldPath = 'admin/security/crate_admin_user_notification';
	$newPath = 'admin/security/create_admin_user_notification';
	$select = $installer->getConnection()->select()
	->from($table, ['config_id', 'path'])
	->where('path = ?', $oldPath);
	$rows = $installer->getConnection()->fetchAll($select);
	foreach ($rows as $row) {
	$installer->getConnection()->update(
	$table,
	['path' => $newPath],
	['config_id = ?' => $row['config_id']]
	);
	}
	}
	$installer->getConnection()->update(
	$installer->getTable('core/config_data'),
	['path' => 'admin/security/create_admin_user_notification'],
	['path = ?' => 'admin/security/crate_admin_user_notification']
	);

it seems easier than selecting then cycling no?

I've tested out both and I can confirm they work

[pquerner]

Leave typos alone. We open a unnecessary hell-hole here.

[fballiano]

so, we've 2 negative vote, I'm kinda neutral and probably @addison74 too, we've to decide something

[addison74]

I would not analyze this case in the general way, "not fixing typos". If we do this it means that we set up a rule for the OpenMage code. I consider this PR to be fine as long as the initial value is corrected by changing it.

Although in such cases there are people who claim that BC could occur, for this PR no one came with a proof. I am for approval, in the form proposed by @fballiano, skipping the loop.

[pquerner]

No one came with proof because the core itself or known 3rd party code doesnt use it. That doesnt make this argument invalid.
We open a hell-hole because the Devs are once in a blue moon (if even) confronted with a typo? Sounds absolutely silly to me.

[fballiano]

I see both of the arguments but I also think we should at a few things

• technically it is a breaking change and it should be treated like that
• specifically this change I don't think would trigger any disaster with custom code -> which is what makes me positive to the change
• fixing this typo doesn't really provide an improved value -> so is it really worth this discussion?

[kiatng]

I think it is still worth discussing. I'd like to propose that we do not fix typo if it is not backward compatible. Another example, if we correct the typo thinking that no one would use it: "tempate_filter" to "template_filter", it will break my projects and I would have no idea why things don't work anymore.

[addison74]

It is obvious that no one can prove that changing such a word mistake in the code will create a BC. In this case, if the change is still requested, it would be natural to anticipate a possible error through a message. In this way, the developer will quickly identify the problem and adapt his code immediately.

[fballiano]

ok it's really not worth fighting over a typo, what @kiatng said makes a lot of sense, it's not worth a BC for a typo, whatever small or big the BC is.
It's not great to have typos in our code but it is what it is, it's very old and there will be some more.

We should have an RFC to decide how to handle these things.

For the time being I'll close this PR.