Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

permit core classes override #860

Closed
wants to merge 1 commit into from
Closed

permit core classes override #860

wants to merge 1 commit into from

Conversation

G3z
Copy link

@G3z G3z commented Mar 21, 2013

with this modification it is possible to write plugins that overrides most joomla core classes, for example JAccess, with a simple plugins like

<?php
    defined('_JEXEC') or die;

    // include_once JPATH_SITE.'/overrides/access.php';
    JLoader::register('JAccess', JPATH_SITE.'/overrides/access.php');

?>

with this modification it is possible to write plugins that overrides most joomla core classes, for example JAccess, with a simple plugins like
```php
<?php 
	defined('_JEXEC') or die;




	// include_once JPATH_SITE.'/overrides/access.php';
	JLoader::register('JAccess', JPATH_SITE.'/overrides/access.php');




		
?>
```
@nicksavov
Copy link
Contributor

Thanks for coding this, Giacomo!

While we’re transitioning to a new integrated tracker, could you report the issue on our current main tracker at JoomlaCode and cross-reference each with a link to other? Here’s the process for reporting on the other tracker:
http://docs.joomla.org/Filing_bugs_and_issues

Alternatively, let me know if you’d like me to create it for you and I can go ahead and do that.

Thanks in advance and thanks again for coding this, Giacomo!

@G3z
Copy link
Author

G3z commented Mar 22, 2013

@nicksavov
Copy link
Contributor

Great; thank you, Giacomo! I'm set this to pending on the tracker, since we have a pull request. I asked one of our reviewers to take a look too.

@nikosdion
Copy link
Contributor

Without wanting to shoot down Giacommo's contribution, I need to express my concern. This kind of change is dangerous. Let's take a practical example with hypothetical versions. Let's say that developer X creates his own JAccess override based on CMS version 3.1.0. Let's assume that a change is introduced in the core in version 3.2.0. Developers will anticipate this change using an if(version_compare(JVERSION, '3.2.0', 'ge') {....}. If the override is in place, though, both the core and the third party code will be broken.

It only becomes worse when the overridden classes are some not used everywhere in the core, making the bug detection more difficult and much less obvious. Third party developers would receive bug reports they cannot reproduce because they depend on third party code which is broken.

Essentially what this patch does is try to legitimise the biggest faux pas in Joomla! extension development: hacking core. It should not be done, for no reason whatsoever.

What needs to be done is probably add "hooks" (I am using a very loose definition) to allow 3PDs to enhance the functionality of core classes without overriding them completely. For example, JAccess could have something like onBeforeGetActions and onAfterGetActions which call something like plugins – again, this is an example off the top of my head, not necessarily something that you can code the way I purport is possible. But this would be the overall idea. Do not override core classes, provide a way to hook on to them and extend them.

@mbabker
Copy link
Contributor

mbabker commented Mar 23, 2013

I'm confused. Your proposal changes the JLoader::import() method (which is tied to jimport('joomla.filesystem.path); type calls) but what you want to do is override core classes by calling JLoader::register(), which is already possible. Using your previous example, the syntax would be:

<?php
defined('_JEXEC') or die;

// include_once JPATH_SITE.'/overrides/access.php';
JLoader::register('JAccess', JPATH_SITE . '/overrides/access.php', true);

Mark Dexter also has some tips in the Joomla! Programming book (see here for one example) about how to override classes. So, basically, it already is possible if you use the API (JLoader::register(), which would work for autoloaded classes) or this trick by Mark.

Keeping Nicholas' concerns in mind, you really shouldn't be hacking the core code for any reason short of a temporary security fix until a proper update can be made, but since this is something you are trying to do, well, here's how to do it.

@G3z
Copy link
Author

G3z commented Mar 24, 2013

maybe a bit of context may help:

i am developping a product based on joomla and i needed ACL based on groups and single users.
to achieve the second part i hacked JAccess to check a custom table where i did save overrides for the user.
i did add one method

public static function userOverrides($userId, $action, $asset = null)

and hacked a core method

public static function check($userId, $action, $asset = null)

to check for overrides before returning a result
I didn't find a better option to have this working on he whole site (site and admin) than hacking jAccess

so there are basically 2 objection to my proposal: 1 technical and 1 "political"

Technical (@mbabker):

I did indeed read about Mark Dexter's article and
the force paramente is true by default so our code is the same.

public static function register($class, $path, $force = true)

point is that this doesn't work since core classes are loaded after plugin classes, and always forced.
my pull request was basically about that (don't force core classes registration).

Political (@nikosdion):

I agree with your concern about future jAccess updates that may be obscured by my jAccess. You can easily resolve that by checking for Joomla version in the plugin and override it or not and also disable the related component if you feel like it.
you suggest to trigger events on each method. which is ok but doesn't solve the problem of possible incopatibilities, but i need to change the result of check i could always create incompatibilities with future versions.
bottom line: i think a Joomla based site is admin's responsibility.

@realityking
Copy link
Contributor

JAccess should be covered by the autoloader (since 2.5 if I recall correctly) so it will never be imported. Are you sure this change is necessary?

@G3z
Copy link
Author

G3z commented Mar 24, 2013

here is my test:
edit index.php form line 54

// echo $app;
echo "<pre>";

print_r(JLoader::getClassList());

echo "</pre>"; exit;

This way i get all registered classes with their file path

with my modification i get

...
    [jplugin] => /Users/g3z/Sites/fwrPortal/libraries/joomla/plugin/plugin.php
    [jaccess] => /Users/g3z/Sites/fwrPortal/overrides/access.php
    [jxmlelement] => /Users/g3z/Sites/fwrPortal/libraries/joomla/utilities/xmlelement.php
...

with standard joomla

    [jplugin] => /Users/g3z/Sites/fwrPortal/libraries/joomla/plugin/plugin.php
    [jaccess] => /Users/g3z/Sites/fwrPortal/libraries/joomla/access/access.php
    [jxmlelement] => /Users/g3z/Sites/fwrPortal/libraries/joomla/utilities/xmlelement.php

(libraries/loader.php:141):

// If we are importing a library from the Joomla namespace set the class to autoload.
            if (strpos($path, 'joomla') === 0)
            {
                // Since we are in the Joomla namespace prepend the classname with J.
                $class = 'J' . $class;

                // Only register the class for autoloading if the file exists.
                if (is_file($base . '/' . $path . '.php'))
                {
                    self::$classes[strtolower($class)] = $base . '/' . $path . '.php'; 
                                        // I propose :
                                        // self::register(strtolower($class), $base . '/' . $path . '.php',false);
                    $success = true;
                }
            }
            /*
             * If we are not importing a library from the Joomla namespace directly include the
            * file since we cannot assert the file/folder naming conventions.
            */
            else
            {
                // If the file exists attempt to include it.
                if (is_file($base . '/' . $path . '.php'))
                {
                    $success = (bool) include_once $base . '/' . $path . '.php';
                }
            }

it's clear that joomla classes are loaded into JLoader:classes without any check while if register is used this happens (libraries/loader.php:216):

public static function register($class, $path, $force = true)
    {
        // Sanitize class name.
        $class = strtolower($class);

        // Only attempt to register the class if the name and file exist.
        if (!empty($class) && is_file($path))
        {
            // Register the class with the autoloader if not already registered or the force flag is set.
            if (empty(self::$classes[$class]) || $force)
            {
                self::$classes[$class] = $path;
            }
        }
    }

and the class is registered only if there isn't already one or if forced.

so basically i could use JLoader::register from your plugins but joomla classes are loaded afterward and so they overwrite your stuff. I'm suggesting to load core classes from library as a fallback and not by force.

@brianteeman
Copy link
Contributor

The related item on the feature tracker has been closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants