It's still in the 3.x branch!? I swear that was removed with 3.0.

On Sunday, November 23, 2014, Nicholas K. Dionysopoulos <
notifications@github.com> wrote:

JParameter had been marked as deprecated. That said, minor releases of
Joomla! 3 are not supposed to break backwards compatibility. Therefore,
removing JParameter (obsoleting it) in a minor release is a critical
backwards compatibility breaking issue.
Steps to reproduce the issue

Install a component which uses JParameter, e.g. Akeeba Backup 4.0.5.
Expected result

The component should work because JParameter is deprecated, not obsolete
(not until 4.0).
Actual result

The component doesn't work because JParameter has been removed.
System information (as much as possible)

Not specific to any configuration.
Additional comments

You should have a JParameter class which extends JRegistry without any
further changes but marked as deprecated.

—
Reply to this email directly or view it on GitHub
#5179.

c7c3722

JParameter was removed from the 3.x code base in that commit from July 2012. It's still correctly in the 2.5 tree with the @deprecated 12.1 statement (thank you Platform versioning).

I've also gone through my local archives of packages since I started with releases last year to verify the file wasn't accidentally included and can confirm the local archives match the git tags.

Hm, then why was my code working? The only other explanation is that JComponentHelper suddenly started returning the raw JSON string of the parameters instead of a JRegistry object. Unfortunately I will be out of office today and cannot check :(=

OK, here's the code with the problem before I head out of the door:

$component = JComponentHelper::getComponent('com_foobar');
if (is_object($component->params) && ($component->params instanceof JRegistry))
{
$params = $component->params;
}
else
{
$params = new JParameter($component->params);
}

Under Joomla! 3.3 $component->params is an instance of JRegistry. Under
Joomla! 3.4 $component->params is JSON text. So I guess the bug is in
JComponentHelper? It's still a b/c break.

There was a change to that method done recently by @Hackwar:
#5124

Maybe related?

Yes. It seems that the changed line 423 is the culprit. It fetches the raw JSON data from the cache.

Revert?

No, the reason is not that it returns a raw JSON, its returning a Registry instead of JRegistry, which you are checking against. I was using JRegistry at first, but was then asked to change it to Registry instead. Looks like we can't change JRegistry to Registry in the 3.x branch.

The respective change was done here: 22d8129#diff-4

No, the reason is not that it returns a raw JSON, its returning a Registry instead of JRegistry, which you are checking against.

That shouldn't matter because we have a class alias set up. PHP will threat them as equivalent.
See https://github.com/joomla/joomla-cms/blob/staging/libraries/classmap.php which is loaded in https://github.com/joomla/joomla-cms/blob/staging/libraries/cms.php#L36.

So as long as libraries/cms.php is loaded, it should work fine.

JComponentHelper::getComponent()->params will always be a Registry object. Just tested that a minute ago.

Unless Nicholas is extending JComponentHelper and has written his own getComponent method, this should properly work.

@nikosdion Are you maybe setting up your own Joomla enviroment within Akeeba Backup where you don't load the cms.php?

Just a comment that may be related. I had a check like:

if ($params instanceof JRegistry)

in one of my extensions and I had to change it after the JRegistry change to:

if ($params instanceof JRegistry || $params instanceof Joomla\Registry\Registry)

I wanted to dig into it before reporting it but I guess this can be related. In that case that would be a B/C issue.

@nikosdion Maybe you should in general replace JParameter with JRegistry. I mean, you are not supporting 1.5 anymore, so that would cover both 2.5 and 3.x and JParameter was deprecated since 1.6.

The issue is not with JParameter. The point is that the instanceof JRegistry check fails for some reason. Which means that the class_alias fails.
If that is the case, then we have a bigger B/C issue with 3.4.

I just tested it:

$component = JComponentHelper::getComponent('com_content');
$params = $component->params;
var_dump($params); //Its a Joomla\Registry\Registry object
var_dump($params instanceof JRegistry); //is false
var_dump($params instanceof Registry); //is false
var_dump($params instanceof Joomla\Registry\Registry); //is true
var_dump(is_a($params, 'JRegistry')); //is false

And that was on a vanilla Joomla staging. So the class alias does not work.

Yes, of course the issue is not with JParameter, but @nikosdion code is still wrong. If for example for some strange reason the params are still not a Registry object, it will still fail because there is no JParameter. So he has to change that anyway.

If the code worked before and it doesn't work now because of a change in
the core code then that is a b/c break which we don't allow.
On 24 Nov 2014 10:12, "Hannes Papenberg" notifications@github.com wrote:

Yes, of course the issue is not with JParameter, but @nikosdion
https://github.com/nikosdion code is still wrong. If for example for
some strange reason the params are still not a Registry object, it will
still fail because there is no JParameter. So he has to change that anyway.

—
Reply to this email directly or view it on GitHub
#5179 (comment).

Changing the code to this: (Notice the first line)

class_alias('\Joomla\Registry\Registry', 'JRegistry');
$component = JComponentHelper::getComponent('com_content');
$params = $component->params;
var_dump($params); //Its a Joomla\Registry\Registry object
var_dump($params instanceof JRegistry); //is true
var_dump($params instanceof Registry); //is false
var_dump($params instanceof Joomla\Registry\Registry); //is true
var_dump(is_a($params, 'JRegistry')); //is true

That makes it work. That means that our class_alias is broken.

@mbabker Can you check why this happens? Maybe something went wrong when I merged 3.4 into staging?

Yes. We need that any check like:

if ($params instanceof Registry)

works properly because it's used by almost every extension out there.

@brianteeman I'm not saying that this is not a break in b/c and that it is @nikosdion fault. I'm saying that besides that, his code is still faulty. Even if we fix this, his code will still reference a non-existing class, which we deprecated 5+ years ago and removed ~2 years ago.

Imho, when Michael did the original PR (#3374) which introduced the class_alias, we left all checks with JRegistry and everything worked. It was changed after that in a following PRs (like #4187) to change the checks to Registry.

Haha, found the issue. You are using JLoader::registerAlias to register the alias of the class. That method however does not directly register the alias, but instead stores it in an array to use in the autoloader. This works fine when you do

$params = new JRegistry;

because it then looks up the class JRegistry, does not find it, runs through all our autoloaders and that is it. In the case of

if ($params instanceof JRegistry)

it does NOT look up the autoloader and load the class first, but only looks at the object, gets the class name and compares it to the string given. Since we never loaded JRegistry before, the internal class list does not know about the alias autoloader and thus returns false.

Long story short: We have to replace the registerAlias() with a direct class_alias(). That actually sucks big time.

To give another example:

<?php
//Assume we only loaded the framework at this point and nothing else
JLoader::registerAlias('JRegistry', '\\Joomla\\Registry\\Registry'); //The alias is registered in our autoloader, but class_alias() has NOT been executed
$params = new \\Joomla\\Registry\\Registry;
if ($params instanceof JRegistry)
{
    echo 'I\'m a JRegistry object!'; //This will not happen!
}

$params = new JRegistry; //Now our autoloader is called and the loadAlias() method sees JRegistry in its cache and calls class_alias
if ($params instanceof JRegistry)
{
    echo 'I\'m a JRegistry object!'; //This WILL happen!
}

We probably have to kill the just in time loading in registerAlias to direct class_alias loading:

    public static function registerAlias($alias, $original)
    {
        if (!isset(self::$classAliases[$alias]))
        {
            class_alias($original, $alias);

            self::$classAliases[$alias] = $original;

            return true;
        }

        return false;
    }

We don't need to even cache the aliases created because that won't be checked again as the class already exists.

So we could just replace the calls inside classmap.php to:

class_map('JRegistry', '\\Joomla\\Registry\\Registry');

instead of:

JLoader::registerAlias('JRegistry', '\\Joomla\\Registry\\Registry');

As I said, I was out of office all day. I just got back. I saw that this code broke last night and I assumed it was the removal of JParameter. In any case, Joomla! 3.4 is supposed to be backwards compatible with 3.3 and it isn't.

BTW, regarding Roberto's comments: no, you cannot use direct class_alias. This would load all classes in memory which would have a major performance impact.

Sorry, guys, but class aliasing cannot be the solution when namespacing existing code. The only reasonable solution is to have a "legacy" layer where old class names extend the namespaced ones and the autoloader can find them. For example:

class JRegistry extends \Joomla\Registry\Registry

It is a dirty solution but doing anything else WILL break backwards compatibility.

Yes. I just tested this further and class_alias() will always load the original class directly. So class_alias() does not simply register that alias and then loads the original class when necessary.

@nikosdion I don't see why you are closing this issue. We are far from done with this...

I meant to press Comment, not Close and Comment. Sorry!

There is an issue with creating a fake JRegistry extending the namespaced Registry.
Your code would break as well since we pass a \Joomla\Registry\Registry object and not JRegistry.

The only way I see is to keep creating JRegistry objects in core code. Then the class will be loaded by the autoloader and everything works fine afterwards. With 4.0 we will be able to change to the framework version.

Or is there a better way?

We can also replace:

use Joomla\Registry\Registry;

with:

use Joomla\Registry\Registry as JRegistry;

And then keep using:

$class = new JRegistry();

I don't think we need to change the use part. Just using $class = new JRegistry(); works already due to autoloader.

for JRegistry, the easy solution would be class_alias(), but I don't know how many other classes we have that we are replacing with namespaced stuff.

True, my code would still break, so you should use JRegistry in core instead of Registry. Changing the object type always breaks b/c.

Maybe you CAN do something else, though. It's a bit far fetched:

• Register class aliases with the autoloader
• If the autoloader is requested to load either the real class (\Joomla\Registry\Registry) OR the alias (JRegistry) the autoloader would a. load the real class and b. set up the class_alias.

The idea is that when you use a class which replaced a JClass (e.g. \Joomla\Registry\Registry) you also get the class_alias('\Joomla\Registry\Registry', 'JRegistry'). Likewise, if someone first uses the old class (JRegistry) you BOTH load the new class AND the alias. There is a minimal performance impact and no memory impact. Bonus: it's a universal solution for all classes without changing our existing code, except for the autoloader.

@nikosdion the issue is, that the autoloader is not invoked when it is simply comparing the object type against a string. It has an object of type "Registry" and we are comparing that (via instanceof) to "JRegistry".

If the autoloader is requested to load either the real class (\Joomla\Registry\Registry) OR the alias (JRegistry) the autoloader would a. load the real class and b. set up the class_alias.

I was just thinking if that would be possible. That would be the most elegant solution imho.

the issue is, that the autoloader is not invoked when it is simply comparing the object type against a string. It has an object of type "Registry" and we are comparing that (via instanceof) to "JRegistry".

The autoloader is executed when the class is created. If the alias is set up as well at that time, then it works.

ah, okay, that would be a possibility.

At this point I think the easiest solution is to keep all the old new JRegistry calls and wait to replace them in v4.0. That should have 0 performance issues.

We are using the new framework libraries that is the most important here.

We need to revert several PRs then or create a new one to replace all new Registry with new JRegistry.

Personally I would prefer a way to setup the alias during class instantiation if it's possible to do without creating performance issues. It would be worth a shot for sure as it would fix similar issues in the future.
If it doesn't work we can revert to JRegistry class creation.

Anyone interested in creating a PR?

How many classes are we talking about that have been aliased? If we are only talking about JRegistry and its subclasses, we could replace that one call to JLoader::registerAlias() with class_alias() directly and be done with it. That would solve the whole issue, but it should be clear that we can't do this with other classes in the future.

Thomas, what is the timeframe? This week is a bit hectic for me and I was trying to write some code to fix JUpdater issues, but I'm willing to give it a shot if nobody else volunteers.

Why don’t we follow this? It seems to me fully B/C for both namespaced and the old code

This is a very complicated way to do exactly what I said. Since we already have an autoloader with aliasing support we simply have to modify it instead of having to reinvent lumbering, the lathe and the wheel ;)=

Fair enough

I provided a possible PR to fix this for now in #5184.

I'd go with #5184 (easy solution) or with a revert of the JRegistry calls (hard solution) as there are only a few classes that we may use and things like JRegistry, etc. will be load into memory anyway.

instanceof does not call autoload so I doubt we can fix this issue inside JLoader.

instanceof does not call autoload so I doubt we can fix this issue inside JLoader.

You can if we use the approach proposed by Nicholas. Every class is once loaded using the autoloader. This always happens before the instanceof check is executed. If the autoloader registers also the alias at the time the original class is loaded, then every works fully B/C.

While the fix from Hannes works for now, we will need another fix possibly in future when we start doing more of this kind.

@Bakual Would you like me to pursue a fix along the lines of my proposal, Thomas?

I'd love to see it, yes. Sent you a mail regarding timeframe.

The problem is, that it will be a somewhat big performance hit. For every class that we load, we have to search through the available aliases and even if we add a reverse lookup table to this, this means a full scan for 98% of all cases where we load a class, simply to make sure that we do not miss an alias. That will be timeconsuming and it will have a negative impact on our system.

Considering the amount of work, I would go the way of #5184 for now and otherwise accept that we can't namespace more classes in the 3.x branch.

In-memory hash table lookups are very cheap, that’s why PHP uses this construct extensively. Not to mention that we have just a couple of hundred calls to the Autoloader in the worst case scenario. I foresee an impact of <1% on our typical pages. Anyway, talking is cheap. Let me write the code and we can benchmark the solution.=

otherwise accept that we can't namespace more classes in the 3.x branch

I won't accept that. Even if it means leaving the un-namespaced classes in place going forward, there is more B/C Framework code than not and it's quite obnoxious that we're still maintaining two sets of the same code. There's ways around everything if you're stubborn enough to find them

There you go. @mbabker Should we add tests as well?

Always add tests :-)

I added tests for the code in JLoader. I'm not sure how I could test the new ClassLoaderJoomla class which is a fork of Composer's ClassLoader with an added call to JLoader::applyAliasFor. Any ideas are welcome.

I like the shuttle test

Just out of interest Nic I presume this worked in 3.3.6? Because this JRegistry class alias existed then? That's what I find most weird about this!

It worked before changing the calls from new JRegistry to new Registry. That's why instanceof is not working now and the JParameter code being executed

Ahh yeah that would make sense - we did that so long ago I'd forgotten about it :)

Fixed with 2e4e486