Improvement: PHP Namespaces support for autoloader, class paths, standard routers

[lamskoy]

Description (*)

This patch allows to code in modern PHP style for openmage using namespaces!

1. Frontend routes. Now you can use Mynamespace\Mymodule\Frontend instead of Mynamespace_Mymodule_Frontend to match app/code/scope/Mynamespace/Mymodule/controllers/Frontend/Class.php Supported namespace inside controller file:

<?php
namespace Mynamespace\Mymodule\Frontend;
class MyController extends \Mage_Core_Controller_Front_Action 
{
}

Module's config.xml definition will be:

<config>
    <frontend>
        <routers>
            <mymodule>
                <use>standard</use>
                <args>
                    <module>Mynamespace\Mymodule\Frontend</module>
                    <frontName>mymodule</frontName>
                </args>
            </mymodule>
        </routers>

2. Backend routes via adminhtml router somespace/some/index URL example Controller file is located under app/code/scope/Mynamespace/Mymodule/controllers/Adminhtml/Class.php

<?php
namespace Mynamespace\Mymodule\Adminhtml\Somespace;

class SomeController extends \Mage_Adminhtml_Controller_Action
{
    public function indexAction()
    {
    }
}

Module's config.xml:

<config>
    <admin>
        <routers>
            <adminhtml>
                <args>
                    <modules>
                        <mynamespace_mymodule before="Mage_Adminhtml">Mynamespace\Mymodule\Adminhtml</mynamespace_mymodule>
                    </modules>
                </args>
            </adminhtml>
        </routers>
    </admin>

3. Models, resource models, helpers, blocks: Mage::getModel() and others are getting possiblity to reslove .php files by namespace path instead of Blablaa_Class_Very_Long_Path;

<config>
  <global>
       <blocks>
            <mynamespace_mymodule>
                <class>Mynamespace\Mymodule\Block</class>
            </mynamespace_mymodule>
        </blocks>
        <helpers>
            <mynamespace_mymodule>
                <class>Mynamespace\Mymodule\Helper</class>
            </mynamespace_mymodule>
        </helpers>
        <models>
            <mynamespace_mymodule>
                <class>Mynamespace\Mymodule\Model</class>
                <resourceModel>mynamespace_mymodule_resource</resourceModel>
            </mynamespace_mymodule>
            <mynamespace_mymodule_resource>
                <class>Mynamespace\Mymodule\Model\Resource</class>

For model you just write in .php file:

namespace Mynamespace\Mymodule\Model;

class Blabla extends \Mage_Core_Model_Abstract
{
}

Model is available like:

Mage::getModel('mynamespace_mymodule/blabla');

Same for helpers, blocks etc.

This change doesn't break any old style definitions and keeps them working

Manual testing scenarios (*)

Questions or comments

This stuff will be useful for custom module developers who bored with long Blabla_Class_Paths :)

Contribution checklist (*)

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

[lamskoy]

I worry that preg_match() has a negative performance impact. Can't we use strpos() instead?

preg_match is cached way better with opcache that strpos. compared some time ago

[addison74]

Maybe this will help you to decide between using preg_match and strpos

http://maettig.com/code/php/php-performance-benchmarks.php https://stackify.dev/280619-preg-match-vs-strpos-for-match-finding

[lamskoy]

Converted to strstr as you suggested

[addison74]

@lamskoy - @woutersamaey requested using strpos function.

Personally I never used strstr function in my code. I would read discussions on forums related to strstr and strpos. It seems most recommend strpos. Here is one discussion:

https://stackoverflow.com/questions/5820586/which-method-is-preferred-strstr-or-strpos

[lamskoy]

@lamskoy - @woutersamaey requested using strpos function.

Personally I never used strstr function in my code. I would read discussions on forums related to strstr and strpos. It seems most recommend strpos. Here is one discussion:

https://stackoverflow.com/questions/5820586/which-method-is-preferred-strstr-or-strpos

Changed to strpos

[luigifab]

If I can say something, do not use "Yoda conditions".

[lamskoy]

If I can say something, do not use "Yoda conditions".

Why? Not so bad stuff as I think :)

[luigifab]

Oh yes, but there are only something like 1% of them in OpenMage.

[addison74]

I agree @luigifab. We must make an effort to maintain the original format. Such an approach stands out immediately especially when it is not used much. There is nothing wrong with the approach but visually I feel that it is different from the rest after years of brushing the code from one end to the other.

[addison74]

Source: https://en.wikipedia.org/wiki/Yoda_conditions.

"Yoda conditions are widely criticized for compromising readability by increasing the cognitive load of reading the code."

I personally did not need their confirmation, but I totally agree.

[lamskoy]

Remade yoda conditions

[addison74]

LGTM

[lamskoy]

Welcome to merge this stuff :)

[addison74]

I would add it to OpenMage-20. It is a feature by which a developer who creates new extensions, I doubt that anyone will rewrite their old extensions on the Magento 2 format. If there are others of the same opinion, the branch must be changed.

[fballiano]

I think this is a good addition but I'd like to have an opinion from the elders :-)

@colinmollenhour @sreichel @tmotyl

[colinmollenhour]

Since we have the assumptions that a class will either contain only '' or only '_' I wonder if something like this would be faster since it would stop scanning after the first match in either case instead of potentially scanning the entire string in the case of a negative.

-        if (strpos($module, '\\') !== false) {
-            $className = $module . '\Exception';
-        } else {
-            $className = $module . '_Exception';
-        }
+       preg_match(/[\\_]/, $module, $matches);
+       $className = $module . ($matches[1] ?? '_') . 'Exception';

If that's not faster then at least flip the if statements so that it checks for _ instead of \ since _ will be the far more common case.

[lamskoy]

If that's not faster then at least flip the if statements so that it checks for _ instead of \ since _ will be the far more common case.

preg_match is slower

[addison74]

There are benchmarks related to preg_match. The conclusions in all it this function is slow.

I personally think there is a very small chance that someone will rewrite their Magento extensions or OM code on this format. Not to mention the developers of new extensions (if they still active) learned with the variant used so many years.

I have the following ambiguity. As far as I can see, there are if statements with strpos function evaluations. If we don't use this format, could it have any impact on performance?

Also, if it has an performance impact shouldn't there be an option in the Backend configuration to activate it when needed and the default value in dropdown list to be No?

[fballiano]

flip the if statements so that it checks for _ instead of \ since _ will be the far more common case.

I think that wouldn't be possible since a classname in a namespace could still have an _ in it right?

[fballiano]

I have the following ambiguity. As far as I can see, there are if statements with strpos function evaluations. If we don't use this format, could it have any impact on performance?

strpos is the fastest function for this kind of things (AFAIK) so there wouldn't be any fastest alternative anyway

Also, if it has an performance impact shouldn't there be an option in the Backend configuration to activate it when needed and the default value in dropdown list to be No?

The impact of the actual code would anyway be lower than the one that checks for the configuration and then executes the code or not ;-)

[woutersamaey]

PHP 8 has str_contains() which may be better. I have put a shim for this function in Mage/Core/functions.php on my installs for people who run PHP < 8.

[fballiano]

From what I can see here: https://github.com/EFTEC/php-benchmarks the performance is pretty much the same, maybe with str_contains the code would be more readable?

[colinmollenhour]

Neither strpos nor str_contains can terminate the search on matching either of the two delimiters, they have to search for one or the other so then best case they find a match early and worst case they scan the entire string to the end and do not find a match. Using preg_match ensures that the full string is not scanned, but the other overhead may outweigh that benefit given the strings are never going to be super long... Other benchmarks are irrelevant so I think a test for this exact condition would be needed to say for sure. preg_match can be very very fast, it just depends on the regex and the input.

However, a no-brainer optimization is to flip it so the _ is the positive case instead of the else case.

I'm on the fence as to whether or not this feature should even be accepted.. Personally I'm comfortable without namespaces in M1 code and it doesn't bother me to keep using the prefix method, but if other users really think it would get some use I'm not opposed to this change. I'm guessing impact would be negligible but certainly there are more cycles consumed for a feature that some users may not ever use.

[luigifab]

Why not for 1.9.4.x. Is the author have a simple module ready to use to test the PR?

[sreichel]

Why not for 1.9.4.x. Is the author have a simple module ready to use to test the PR?

Just pushed some files together to test autoloading ....

composer config repositories.openmage-namespace-test git https://github.com/sreichel/openmage-namespace-test.git
composer require openmage/namespace-test:dev-master

LGTM so far. :)

[lamskoy]

Wow, thanks for a test module

[sreichel]

Wow, thanks for a test module

I REALLY like this feature :)

Feel free to extend this test module.

[fballiano]

@sreichel what about https://github.com/OpenMage/magento-lts/pull/2300? which one should we choose?

[sreichel]

@sreichel what about #2300? which one should we choose?

#2300 has no description, so i even dont know the purpose of it ....

[fballiano]

@sreichel what about #2300? which one should we choose?

#2300 has no description, so i even dont know the purpose of it ....

sorry my fault, i thought it was about autoloading but not exactly