Please remember to adjust the tests from #66 once they are merged. With this PR, direct access of the separator property MUST NOT be used anymore.

Okay guys, on second thought I decided not to do a magic _get/_set.
This is to much effort with a litle gain and high chance to break some poor CMS extension.

Example when any extension creating a dynamic property , after adding __get it become not possible, and could lead to broken extension.

Originaly I coded this:

 /**
     * Magic method to access separator property.
     *
     * @param  string  $name  The name of the property.
     *
     * @return string|null A value if the property name is valid, null otherwise.
     *
     * @since       2.1.0
     * @deprecated  4.0  This is a B/C proxy for deprecated read accesses
     */
    public function __get($name)
    {
        switch ($name) {
            case 'separator':
                \trigger_deprecation(
                    'joomla/registry',
                    '2.1.0',
                    'The $separator parameter will be removed in version 4.',
                    self::class,
                    self::class
                );

                return $this->separator;

            default:
                $trace = \debug_backtrace();
                \trigger_error(
                    \sprintf(
                        'Undefined property via __get(): %1$s in %2$s on line %3$s',
                        $name,
                        $trace[0]['file'],
                        $trace[0]['line']
                    ),
                    E_USER_NOTICE
                );

                return null;
        }
    }

    /**
     * Magic method to access separator property.
     *
     * @param  string  $name   The name of the property.
     * @param  mixed   $value  The value of the property.
     *
     * @return void
     *
     * @since       2.1.0
     * @deprecated  4.0  This is a B/C proxy for deprecated read accesses
     */
    public function __set($name, $value)
    {
        switch ($name)
        {
            case 'separator':
                \trigger_deprecation(
                    'joomla/registry',
                    '2.1.0',
                    'The $separator parameter will be removed in version 4.',
                    self::class,
                    self::class
                );

                $this->separator = $value;
                break;

            default:
                if (property_exists($this, $name)) {
                    throw new \RuntimeException(
                        \sprintf(
                            'Cannot access protected or private property %s::$%s',
                            __CLASS__,
                            $name
                        )
                    );
                }

                $trace = \debug_backtrace();
                \trigger_error(
                    \sprintf(
                        'Undefined property via __get(): %1$s in %2$s on line %3$s',
                        $name,
                        $trace[0]['file'],
                        $trace[0]['line']
                    ),
                    E_USER_NOTICE
                );

                $this->$name = $value;
                break;
        }
    }

But after testing I found that next code will be broken after introducing magic:

$params->potato = 1;
var_dump($params->potato);

It appears that the Registry does not currently have a magic setter or getter, and setting any class value should not work or be done. The fact that setting values like $params->potato = 1; works is actually a side-effect and places the value outside the actual registry.

Array access, however, is a feature due to ArrayAccess, and allows for the setting and retrieval of values. Therefore, we should only allow the separator value to be set or retrieved via the magic methods, and any other value should throw an exception.

If adding new magic methods will be a breaking change, we can deal with it in another PR. This PR was originally intended to allow for setting up the separator with the constructor, which seems all good.

The Object access is an side-affect, not a feature.

$params->potato = 1;
var_dump($params->potato); // returns int(1) 1

That is why this for example returns null:

$reg = new \Joomla\Registry\Registry();
$reg->potato = 1;
var_dump($reg->get('potato')); // returns null

What does work is array access (as a feature), because of the ArrayAccess interface being honored:

$reg = new \Joomla\Registry\Registry();
$reg['potato'] = 1;
var_dump($reg['potato']); // returns int(1) 1
var_dump($reg->get('potato')); // returns int(1) 1

So that mean this returns the actually value from inside the registry

It even honors the separator:

$reg = new \Joomla\Registry\Registry();
$reg['name.key'] = 'name';
var_dump($reg->get('name.key')); // returns string(4) "name"
var_dump($reg->get('name')); // returns object(stdClass)#580 (1) { ["key"]=> string(4) "name" }

I would like to ask for the opinions of @wilsonge, @nibra or @HLeithner regarding dealing with these changes, what version should we target, and if @Fedik still not willing to add the magic methods will one of you open a PR for it? or what do you think should be done here?

more about adding this can be found here #58

As stated in a comment on #58, object access is not desired. It is not part of the contract.

$reg = new \Joomla\Registry\Registry();
$reg->potato = 1;

has never been allowed.

has never been allowed.

Yes and no 😉
Yes, it is not provided by Registry, but It is a default feature of PHP, here we cannot say "never".

However it planed to be removed in php 9, as far as I know.

I just want to reduce a risk of break something untill then. Because Registry often used for $params object, and this "may" be used incorrectly by some extension.

I just want to reduce a risk of break something untill then

Valid point. So issue a deprecation warning on object access for any value, and a special one for separator. Then everybody can prepare for 4.0.

I recall #58 being previously blocked, has our stance on it changed?

I personally like the object access from #58 and providing more ways to access data, as PHP is my preferred language. However, I am also content with a standard method and array access as being sufficient. Therefore, I do not wish to see object-like access added to the Registry at this point, as stated in the #58 thread. The two current access methods are already well-documented.

If our opinion changes, I would not object. However, these changes do conflict with current best practices (which is to add restraint) of the industry. Since restrain on this low level is always seen in a good light from what I have understood at least... though not all agree obviously. Will they ever?

So I would agree with @nibra that we should not change the access to that of #58 but still apply the patch as suggested by @wilsonge

Added magic stuff

We can't forbid to set properties in the magic getter and setter this would be a b/c break because now it's possible, so we have to do the same for all variables like for the separator and throw a deprecation warning that direct class variables access will not be allowed starting with 3.0,

this will also effect Joomla 5.0 which would be off course a b/c break for unintended behavior. Which shouldn't effect too many extensions. at least we have to document this also in manual.joomla.org

Yes this I can support, and this makes sense.

So I change removing version from 4 to 3?

Yes, please.

So for clarity, we allow setting and getting of class properties with deprecation warning that direct class variables access will not be allowed starting with 3.0 (Keep the \RuntimeException( for private and protected in the getter method)

Yes, please.

So for clarity, we allow setting and getting of class properties with deprecation warning that direct class variables access will not be allowed starting with 3.0 (Keep the \RuntimeException( for private and protected in the getter method)

true private and protected function should not be available thru the magic functions.

@Fedik a deprecation is not a fatal error, so we shouldn't trigger this exception for php 7.2

I cannot check currently on 7.2, but on 8 I have an Error throwed when trying access protected property var_dump($registry->data).
Can someone check what happen on 7.2?

I cannot check currently on 7.2, but on 8 I have an Error throwed when trying access protected property var_dump($registry->data). Can someone check what happen on 7.2?

yes because $this->data is protected here we have to throw the exception as @Llewellynvdm mentioned.
but for $this->nonExistingVar we have to allow the access (read/write)

Can someone check what happen on 7.2?

Sorry I can't check this right now, can you try using a old docker container?

I have updated,

but for $this->nonExistingVar we have to allow the access (read/write)

That should work, at least it working for me :)

$a = new Registry;
$a->b = 1; // Works
var_dump($a->b);  // Works
$a->b = 2; // Works
var_dump($a->b);  // Works

$a->data = 3; // Here an exception
var_dump($a->data); // Here an exception

Note: The magic methods does not called for existing public properties.

We can't forbid to set properties in the magic getter and setter this would be a b/c break because now it's possible

That has never been part of the contract, so it is NOT a B/C break. Being able to set properties directly runs counter to the purpose of the registry.

I hear you, but the promise is not just what is documented but also what is possible, as the code is part of the "document." Since the Registry allows this behavior without warning or any kind of notice, it will not be hard to prove that it is, in fact, part of the promise at this time.

It will not help anyone to just break the code of those using it downstream. Giving them a heads-up is best practice.

Yes the Registry at this time, without this PR, does in fact allow:

$reg = new \Joomla\Registry\Registry();
$reg->potato = 1;
var_dump($reg->potato); // returns int(1) 1

Yes the Registry at this time, without this PR, does in fact allow: [...]

Yes, that's a design failure. If that was allowed, it had to be included in toArray() and consorts, which it isn't.

Design failure or not, we will have to give people a heads-up. Breaking things when you fix things without warning has never been a good experience for anyone.

I don't understand why you would follow any other path, its not like we are adding the behavior we are in fact taking steps to repair the design failure in a none destructive way.

Being able to set properties directly runs counter to the purpose of the registry.

This part of PHP behavior, it is there even if we do not want it 😉
But as already noted it is marked for deprecation and will not be possible in PHP 9 (at least by default)

I have added it to __set to reduce a chance to break something, when someone use this PHP feature.

LGTM, however, maybe deprecation should be targeted at 4.0, since 3.0 is quite close to be released?!

I am fine with any version :)
I can change to what you guys decide

LGTM, however, maybe deprecation should be targeted at 4.0, since 3.0 is quite close to be released?!

I don't see a real reason to delay this, most of it is unintended behavior anyway.