Skip to content

Conversation

@korenevskiy
Copy link

@korenevskiy korenevskiy commented Oct 27, 2021

Add magic methods.
The Registry class is used to get data of different configurations and settings.
In the case when we want to get configuration data with a non-existent setting, we want to use the default value.
But when we use regular objects, we get a PHP error about missing a property.
And when using the Get(), Set() methods, we have to write a lot of code, the text becomes NOT susceptible to reading.

$array = ['id'=>123, 'name'=>'Text Button'];
$params = new Registry($array);

The old syntax.
echo "<button id='" . $params->get('id') . "'>" . $params->get('name') . "</button>";
New syntax.
echo "<button id='$params->id'>$params->name</button>";

This ability can be used in templates of modules, plugins, components.
You can see how much easier it is to write?
And much more important that such code is easier for programmers to read again.

In the description of this class we see:

// Set a value in the registry.
$registry['foo'] = 'bar';

// Get a value from the registry;
$value = $registry['foo'];

// Check if a key in the registry is set.
if (isset($registry['foo']))
{
	echo 'Say bar.';
}

This is a similar approach.
But now we can write:

// Set a value in the registry.
$registry->foo = 'bar';

// Get a value from the registry;
$value = $registry->foo;

// Check if a key in the registry is set.
if (isset($registry->foo))
{
	echo 'Say bar.';
}

@korenevskiy
Copy link
Author

korenevskiy commented Oct 27, 2021

in class Registry is property public $separator
Can this property be made private?
And we implement the installation of this property through methods?

@korenevskiy
Copy link
Author

@mbabker @SharkyKZ
What didn't you like about my PR?

@mbabker
Copy link
Contributor

mbabker commented Oct 28, 2021

  1. No tests validating behaviors
  2. Use through object accessors is limited and cannot support nested keys (which means there is not feature parity with the existing API). Plus, the class already implements ArrayAccess so you can work with it using array syntax already.
  3. Making the Registry invokable as a shortcut for binding data is an extremely confusing
  4. In general, this PR brings no quality of life improvements and IMO just adds the potential for more confusion when using it; explicit APIs should always be preferred over magic behaviors, especially when the magic behaviors cannot expose the full API featureset.

@korenevskiy
Copy link
Author

korenevskiy commented Oct 28, 2021

  1. No tests validating behaviors
  1. The new syntax does not break the behavior. Since the new behavior has not been used yet.
  1. Use through object accessors is limited and cannot support nested keys (which means there is not feature parity with the existing API). Plus, the class already implements ArrayAccess so you can work with it using array syntax already.
  1. The new syntax works great with delimiters.
    $params->{'parent.child'}; You will say that this is analogous to accessing as an array. You're right, it's very similar. But this is still access as a property of the object and therefore has uniformity.
  1. Making the Registry invokable as a shortcut for binding data is an extremely confusing

You are right, the magic method __invocable has questionable behavior. I will remove this method.
Using access to parameters as properties of an object is normal behavior for object-oriented programming.

echo "<button id='$params[id]>$params[name]</button>";
or
echo "<button id='$params->id'>$params->name</button>";
The new syntax reads better than if you read the syntax with access as an array by key. or access by the Get() method.

  1. In general, this PR brings no quality of life improvements and IMO just adds the potential for more confusion when using it; explicit APIs should always be preferred over magic behaviors, especially when the magic behaviors cannot expose the full API featureset.

Let's analyze it.
in 95%-99% of cases, access to the Registry object is used without applying default values.
I.e. in most cases, key access is a simple operation.
But due to the fact that the syntax has access as an array by key or as a method call, it is difficult to write and read code.
When writing a key to access an array, you need to specify square brackets, and in the interior you need to specify quotes. Which is similar to specifying quotation marks when writing the Get() method;
I will mention again that I agree with you that I wrote the __invocable() method in vain, this will add confusion.
But in all other respects, the new syntax has no restrictions, does not introduce confusion, improves code reading, improves code writing.
Improves the writing of code in different ways of writing.

  • It is convenient to write code inside the echo ""; method
  • It is convenient to write code by itself according to the methodology of object-oriented access.
  • Improves code reading in all cases.

And instead of disliking, you could explain what exactly you liked and what exactly you didn't like.
Since there may be individual mistakes in any PR, but the PR itself can be of great benefit. And you, without explaining your expert experience, did not try to improve my idea and point out my mistakes.

@korenevskiy
Copy link
Author

korenevskiy commented Oct 28, 2021

@mbabker
I have corrected the PR according to your corrections.

@mbabker
Copy link
Contributor

mbabker commented Oct 28, 2021

  1. No tests validating behaviors

The new syntax does not break the behavior. Since the new behavior has not been used yet.

On the contrary, that's exactly why tests need to be added. You've added new methods to the Registry class as you've introduced a new behavior for the class (reading and writing registry keys using PHP object syntax). Sure, these new magic methods are wrappers to existing class methods, but tests should still be added to help ensure that the behavior is not broken later. The ArrayAccess implementation is functionally equivalent to these magic methods, and that is tested to ensure that $foo = $registry['foo']; does not break (basically ensuring it always is the same as $foo = $registry->get('foo');).

The new syntax works great with delimiters. $params->{'parent.child'};

At that point, does that really read any better than $params['parent.child'] or $params->get('parent.child')? I would say no as it's adding cognitive overhead in reading the code, you have to understand much more of PHP's parser logic when you introduce the brackets there. I'd take array access over object access in this case.

And instead of disliking, you could explain what exactly you liked and what exactly you didn't like.

I just don't like it. It's not that this PR is fundamentally flawed or incorrect in any way. That's just my personal taste of how I write PHP these days, I prefer code that reads clearly over code that relies on magic behaviors. In any of my own projects, I'd personally reject a patch that would try to use the Registry class with either object or array syntax over the class methods themselves; I think it is easier for folks to grasp that you are calling a named method on a specific class than it is to try and explain (then remember later on) that the class exposes features which allows you to use array syntax or to read and write data like you're writing to properties on the object, and that code will always be clear in context no matter what happens later on. I also realize that some folks like the convenience of these syntaxes when working with class objects, so for those who like it then that's good for them.

@korenevskiy
Copy link
Author

korenevskiy commented Oct 28, 2021

@mbabker

Writing functions with curly braces can really cause cognitive problems.
But I will note the following:
Since in 99% of cases, the properties of an object do not have a separator in the name. Then such corrections are only for the benefit of the remaining 1%, the programmer is not obliged to use curly brackets, but can use a method call.
I personally would. In 95%-99% of cases I would use it as an object with properties, but only in 1%-5% I would call a function. After all, this is the syntax for everyone to choose according to their taste. How to write code in 1% of cases.
.
Calling the GET method, I have to think every time that the REGISTRY class stores data in $this->data.
But in fact, the behavior of a class always has the same behavior as a regular class, 99% I don't need to think about where and how data is stored there.
This is what the REGISTRY class was invented for. We caller to it by analogy as a Database. And that should be good.
By definition, any Framework is invented in order to relieve cognitive tensions. By simplifying popular cases to a simple one.
If a programmer needs to write code straining his head in 5% of cases. Then this does not mean that we should make the framework so that in other cases it necessarily strains the head.

@korenevskiy
Copy link
Author

korenevskiy commented Oct 28, 2021

@mbabker
I don't have the ability to write tests.
But since Joomla uses calling values everywhere by calling a method. This means that the work will not be disrupted.
We can search by keyword in the Joomla code implements Registry .
We will see that no class inherits from the Registry, which means the class is compatible.
But this is already a compatibility task. And is not the task of a bad idea.

@korenevskiy
Copy link
Author

In fact, it is not clear what to write in the tests.
I tested a new class in a working Joomla with the Joomshopping component installed. The site was working.
The test is needed to identify errors. If the test does not detect errors, then such a test is useless.
You can write tests with typed data as much as you want to check that a number is a number and a string is a string. But if the data is typed, these tests are useless.

@mbabker
Copy link
Contributor

mbabker commented Oct 28, 2021

Calling the GET method, I have to think every time that the REGISTRY class stores data in $this->data.

No, you don't. Or, rather to rephrase things, why do you need to understand the internals of the Registry class when calling its methods from the outside? The API contract is you call the get method with specific parameters and it has an expected return value (either a value from a key in its internal data store or whatever you specify for the "default" parameter, which defaults to null). You shouldn't need to care that $this->data is a thing or whether it stores values in an object structure or an array structure or things like that. That is really only a concern if you're subclassing things.

But since Joomla uses calling values everywhere by calling a method. This means that the work will not be disrupted.

The existing automated tests in this package give 0 coverage to using the object-oriented syntax. Therefore, tests are needed to ensure that this feature is covered and is not accidentally broken in the future. The entire point of automated testing is to help give developers confidence in the code by having their API features tested and that every feature does not need to be tested by a human with every change (unlike in Joomla world where it seems development is driven by humans validating things in a web browser, I constantly work in applications where I very rarely open my web browser when working on things; these applications have literally thousands of test scenarios covering an overwhelming majority of the code which allows us to make changes safely and lets us know pretty quickly if something is potentially broken). The tests here aren't necessarily to explicitly check values, they're to make sure that feature of the API does not break.

final class RegistryTest extends PHPUnit\Framework\TestCase
{
	/**
	 * Test based on existing `testGetOffsetAsAnArray` test case
	 */
	public function testGetValueThroughMagicGetter(): void
	{
		$instance = new Registry(['foo' => ['bar' => 'value']]);

		$this->assertSame('value', $instance->{'foo.bar'}, 'An assigned key should be retrieved with object access.');
		$this->assertNull($instance->{'goo.car'}, 'An unknown key should return null with object access.');
	}
}

That is all that is needed to test the new __get() method. And that ensures that retrieving values using object syntax does not break in the future. You'd do similar for the other magic methods (essentially you're duplicating the test cases for ArrayAccess and changing them to object syntax instead of array syntax since the magic getters and ArrayAccess interface are essentially one in the same as far as functionality goes, it is just literally a difference of object syntax versus array syntax).

@korenevskiy
Copy link
Author

korenevskiy commented Oct 28, 2021

@mbabker
Automated testing is of great importance in large teams of developers. In which one application is programmed by many developers.
In such commands, the quality of the application (stability) is much higher than the speed of development.
I'm a lone developer, I don't have any experience in writing tests at all. To write a test, I need to study testing from the very basics. And this is just for making one fix to Joomla which I just love. If I do this, then my PR can be postponed for a very long time.
You say that I'm paraphrasing the meaning of the Registry class.
Or maybe I'm not prefrasing, but just showing you this class from a new angle, from the angle of a single developer.
Calling the Get method with parameters is of course necessary.
I'll tell you.

  1. Making my amendments does not oblige you to use them instead of the GET method. This is analogous to how the ENUM type in PHP 8.1 is introduced. Sugar, for the sake of speed. It was good without him, but it was even better with him in specially defined conditions. This does not mean that it should be used everywhere. I just added magic methods in my PR, while I did not remove the GET, SET methods.
  2. I wrote you an example in the description of the changes
    echo "$params->name";
    But after all, Joomla is used as a startup, few people use it to create corporate solutions.
    There is 1 module developer for every 10 layout designer. And for every 10 module developers, there is 1 component developer.
    You prove to me that it is not worth taking into account module developers and layout designers at all. After all, they are stupid in development and programming

You look like a developer who makes applications that are able to work on his expensive smartphone. And in the calculation of pensioners, schoolchildren, housewives do not take into account at all who have a very cheap smartphone.
I used to have a lot of experience in C# development, and then I wrote software for the servers. But a few years ago, I had to design pages. And I realized that a layout designer needs serious tools.
I can assume that you have very little experience in layout design.

@korenevskiy
Copy link
Author

korenevskiy commented Nov 2, 2021

Hello @mbabker . Could you run this test?

final class RegistryTest extends PHPUnit\Framework\TestCase
{
	/**
	 * Test based on existing `testGetOffsetAsAnArray` test case
	 */
	public function testGetValueThroughMagicGetter(): void
	{
		$instance = new Registry(['foo' => ['bar' => 'value']]);

		$this->assertSame('value', $instance->{'foo.bar'}, 'An assigned key should be retrieved with object access.');
		$this->assertNull($instance->{'goo.car'}, 'An unknown key should return null with object access.');
	}
}

@HLeithner
Copy link
Contributor

I checked the opcode needed to access the object with ->xyz and ->get('xyz') syntax, not much different except the function call is hidden in the magic method.

If you like to use fast_concat you can also do this with the get function or the array syntax. "text {$x->get('var')} more text".

On the other side we have already 2 ways to access the values of the registry and normally an object get accessed with arrow functions.

So I personally see no real benefit (except my personal preference to use objects with the arrow syntax) but I also see no negative consequences by adding it.

To summaries this, if you add the tests correctly, I'm not against this.

I think Michael mean you should duplicate https://github.com/joomla-framework/registry/blob/2.0-dev/Tests/RegistryTest.php#L458-L497 and adapt it for the arrow syntax. Maybe I missed a test.

src/Registry.php Outdated
*
* @return mixed The value of the that has been set.
*
* @since 1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot be correct.

src/Registry.php Outdated
*
* @since 1.0
*/
public function __set(string $path = '', $value = null): void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a default string will allow this to be called without a path which kinda defeats the object of the method right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Only without default value for path attribute.

src/Registry.php Outdated
*
* @return mixed The value of the removed node or null if not set
*
* @since 1.6.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.6.0?

src/Registry.php Outdated
*
* @return boolean
*
* @since 1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.0?

src/Registry.php Outdated
*
* @return mixed Value of entry or null
*
* @since 1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.0?

@korenevskiy
Copy link
Author

korenevskiy commented Nov 21, 2021

@HLeithner @PhilETaylor
I added tests and changed the version of the new methods to 2.2.
I hope I did everything right.
I find it difficult to put up a version.
Since version 2.1 has already been released, so I put version 2.2, but the drone outputs an error.
And if I install version 2.1, then there are no errors.
Which version should I put in the methods?

@nibra
Copy link
Contributor

nibra commented Nov 21, 2021

Which version should I put in the methods?

__DEPLOY_VERSION__ is the right value to use for "whatever release this code will get in to"

@korenevskiy
Copy link
Author

@nibra

__DEPLOY_VERSION__ is the right value to use for "whatever release this code will get in to"

Excuse me. The question is off topic.
And this constant is interpreted as a version by which program? Is this constant interpreted by GITHUB or by an external collector?

@HLeithner
Copy link
Contributor

in the cms the release script changes the magic string to the version it's building. in the framework jenkins does this on tagging

@nibra
Copy link
Contributor

nibra commented Jun 19, 2022

As charming as this approach may be at first glance, it is unfortunately also unsuitable.
The registry class itself has internal properties whose names could no longer be used as keys in the registry: data, initialized, separator.

What should happen for

$reg = new Registry(['data' => ['key' => 'value']]);
$data = $reg->data;

?

For this reason I have to close this PR, but I would like to thank you for your effort and dedication.

@nibra nibra closed this Jun 19, 2022
@korenevskiy
Copy link
Author

korenevskiy commented Jun 21, 2022

For this reason I have to close this PR, but I would like to thank you for your effort and dedication.

@nibra Let's bet.
Property ->data has a private identifier. This is a hidden property and this property cannot be accessed by contacting directly. In this way.
property ->data is not a reason to close PR .
Only one property remains that can be the cause ->separator.
But this property is little used. And you can not take it into account.
In future versions of Joomla, the property ->separator can be assigned to functions.
Your arguments have the truth only by 40%.
There is not a single code in CMS Joomla that uses the ->separator property!
I ask you to open this PR back, and let's look for a solution that will satisfy everyone.
Perhaps we should define the property ->separator as @deprecated and suggest using the method ->setSeparator() and
->getSeparator()

@korenevskiy
Copy link
Author

@nibra
all properties have access similarly as an array.
but if you write $registry['data'] or $registry['separator']
It turns out according to your logic, you must prohibit access to properties as well as to an array.

Leaders who have already watched this PR understand that ->data is a private property. And also the addition does not violate backward compatibility.
The new PR will never spoil the code of existing extensions and programs.

@korenevskiy
Copy link
Author

korenevskiy commented Jun 21, 2022

@nibra

$reg = new Registry(['data' => ['key' => 'value']]);
$data = $reg->data;

There are no contradictions

$data == ['key' => 'value']

To take into account the property ->separator, I will add the condition to the magic method ->__SET() so that the property
->separator is not redefined in the array ->data.
But for this you need to open the PR back. After which the new PR will be fully 100% backward compatible.
After which we will get more and more new functions and everything will work in any environment.

@nibra
Copy link
Contributor

nibra commented Jun 21, 2022

Even if it works with data and initialized - it is not viable to exclude separator from valid key names.

If you absolutely want to call the properties using the arrow, use Registry::toObject() to get the representaion that you want.

@korenevskiy
Copy link
Author

korenevskiy commented Jun 23, 2022

Even if it works with data and initialized - it is not viable to exclude separator from valid key names.

If you absolutely want to call the properties using the arrow, use Registry::toObject() to get the representaion that you want.

I did not propose to exclude the separator now. I suggested deleting the separator in the future version of Joomla 5.
By replacing this property with a call function. But such a replacement must be accompanied by a vote by leaders. This is a separate topic. Now we are talking about supporting the challenge of the properties of the object.
But even if there is a separator in the form as it is now, this is not a problem for support ->property.
I know very well how JRegistry->toObject() works .
I wrote very complex extensions and I can say that using JRegistry->toObject() is painful.
because this method creates a new object. Then you need to change the values of the fields, and for this you need to change the values of the fields immediately in 2 objects.
And also in the object from JRegistry->toObject() there will be no processing of the default values.
All developers use all parameters: modules, components, plugins, fields, extensions always use the JRegistry type.
Calling each property of this object only 5% requires a different default value, in 95% of cases it is always enough to have the default value NULL .
Why all developers need to write a function call for these purposes ->get('property')?
I already wrote this above, and wrote that it will be easier for developers to write code, the code will be readable.
Example:

$module = new JRegistry('{"id"=123, "content"="Content","title"="New Module"}');
echo"
	<label>$module->title</label>
	<div>$module->content  $module->user</div>";

The code is very easy to read. We can write a lot of examples with very simple and readable code.
We see that the USER property does not exist. But the absence of the field does not cause an error, since there is a default value of NULL .
This can very often happen to CHECKBOX`s when the checkbox is deactivated, then the data of this checkbox does not get into the request at all. After which the request is serialized. And when you call the register, and paste the checkbox value into the layout, an error will be caused, since there is no processing of the default value.

@nibra
Copy link
Contributor

nibra commented Jun 29, 2022

We discussed this feature at the last Production Department meeting. The unanimous opinion is that another access method is not desired as it would cause more confusion than solve problems.

@korenevskiy
Copy link
Author

korenevskiy commented Jul 3, 2022

I was waiting for such an answer from you, and you sent this answer.

Quote:

We discussed this feature at the last Production Department meeting. The unanimous opinion is that another access method is not desired as it would cause more confusion than solve problems.

@mbabker @HLeithner @PhilETaylor

Funny, are you seriously saying this? or are you kidding?
Dear colleagues, if you get acquainted with this PR, then the current answer "nibra" complements the previous answers, after which it becomes very funny.

Dear participants - Joomla executives.
I want to complain about "nibra".
It's been 2 weeks since he blocked this PR.
He explained this by the fact that there are 2 members in the JREGISTRY class that will conflict.
But this is not the case at all. This class has an important member, but it is private.
Thus, the reason for closing the PR is completely wrong. Since the second member that can cause closure is not important.
Referring to the reason for closing the PR due to the DATA member is completely wrong.
The JREGISTRY class has 2 members, but each member does not have a weight of 50%. The DATA member has an importance of 99.5%. and the SEPARATOR member has a weight of 0.5%.

I reported this to "nibra", I thought that he either did not understand the code and closed it by mistake PR, or "nibra" has little experience.
I asked him to open the PR back.
After which it became clear that "nibra" has a very large experience in the development, and also "nibra" has a good idea of how this class works. As it turned out, he has experience with Mambo 4.6
Until now, PR is still closed.
PR is fully backward compatible.
Before, I was guided by the rule that if this PR improves the lives of 99% of developers, it will be good for everyone.
But now I am in a different situation.
I am in a situation that if there is at least one developer in the world who will be against this PR, then this is an occasion to reject it.
It is obvious that in the world there will always be 1 person who will be against everything.
I tried to explain "nibra" the reasons for the use of this PR, completely retelling the arguments that I wrote at the very beginning of this PR .
Perhaps there are problems (this is only 1 problem) this PR, how to manage a member of the separator.
This problem is completely solvable by the conditions in the code. But this must be discussed. "nibra" does not want to discuss.
I can’t make corrections to the blocked PR that we can make.

General reasoning :
This situation resembles a situation where Microsoft deliberately slowed down the development of OS Linux. Very much.
As an example, we know that OS Linux has Office Libre (Open Office). This is a very complicated program. While Libre Office is an open program.
We can ask ourselves: Were there and wanted people in the world who could modify the user interface to the mind so that it was not as nasty and bad as the Windows 95 interface.
Despite the fact that there were people who developed the complex LibreOffice. Of course there were such people, since making the OSLinux glamorous interface is a much simple task than creating LibreOffice.
OSLinux needed to add shadows to the windows, and make rounded windows, make the windows wider frame so that it was easier to stretch the windows. You just had to add fixes to the current interface.
The Linux interface looked as if people specifically made it very bad and nasty from the age of 95. I think that Microsoft executives know that if you can’t forbid, then you need to lead a group of people.
Of course, there were any developers who wanted it, this did not happen for obvious reasons, then OS Linux would become a competitor to Windows on home PCs.
Today in 2022, OSLinux began to have a modern interface, but this has happened over the past 2-3 years.
This happened after China began to develop its closed DeepinOS (based on Linux). After which Microsoft is no longer able to slow development Linux. After that, you can notice all Linux`s began to actively develop interfaces in a modern look. Literally at 1-2 years, all Linux became impeccably beautiful. As you might guess, the reason for this was the impetus: the fact that Microsoft cannot slow down the development of DeepinOS .

In open respositories there are always a small part of managers who are interested in slowing development.
I am sure that "nibra" is a very professional developer, and "nibra" is a very attentive leader.
Then why did he block this PR for no reason?
"nibra" refers to very dubious reasons. More precisely, it simply selects the most possible reasons that may be the reason for canceling PR. Even if PR will benefit an order of magnitude more.
And then "nibra" didn't want to open it back.

I understand that complaining about Joomla executives is bad with experience for more than 10 years, but the reason for blocking PR is wrong.
Despite the fact that "nibra" had to block PR, but for no reason it was impossible to block. "nibra" indicated a reason that could only be like truthful. And this is a good reason.
I ask the help community to resolve the conflict. Is it possible to do this?

"nibra" says magic methods can create confusion. It is not true. And also I can say with confidence that magical methods, on the contrary, remove confusion. Since when developing extensions we turn to different objects :
JTable, $module, $params, $plugin. Each of these objects creates confusion. Since the JTable object needs to be accessed by the name of the property, the objects $module and $plugin have the type stdClass also need to be contacted by the name of the property.
But at the same time, the configuration object with the type JRegistry does not allow you to handle the property name. It turns out so that half of the extension code uses property names, and half calls the function ->get('prop'). While the default values of 99% are required blank. Since magical methods can create confusion?if the transition to magic methods will completely facilitate the writing of code, readability of code, layouting of code.

Maybe it has to do with politics?

@PhilETaylor
Copy link
Contributor

PhilETaylor commented Jul 3, 2022 via email

@korenevskiy
Copy link
Author

korenevskiy commented Jul 3, 2022

Absolutely nothing to do with me!

Your comment is on top.
Would you like to use access to JRegistry parameters by property name?

@nibra
Copy link
Contributor

nibra commented Jul 3, 2022

I'm sorry that this is hitting you so hard. As I had feared a reaction of this kind, I put the issue up for discussion in the Production Department - not a single advocate was found. Nevertheless, I will try to give you my reasons for rejecting this PR.

Joomla is a system with a long history, and although the architecture has already been cleaned up to a great extent with Joomla 4, the process is far from over. Some legacy issues such as the use of magic methods or stdClass make it difficult to make the software less error-prone.

Magic methods that respond to arbitrary keys cannot be provided with type hints. This undermines an important instance in quality checking, static code analysis. If an explicit get() method is used, at least this fact is not obfuscated and the developer is not lulled into a false sense of safety. I would also like to remove the array access from the registry, but it has been in there too long for that to be realistic.

Even if such constructs are still present in the current code base, we need to eliminate them one by one. In any case, we should not introduce any new ones with today's knowledge. This will only cause pain in the long run in the development of the overall system.

I hope this makes it clear that a) this is not a solo decision on my part and b) there are indeed factual reasons.

@korenevskiy
Copy link
Author

korenevskiy commented Jul 3, 2022

I realized that there are objective reasons. Based on the stated reasons. I can make a conclusion. That you are trying to get away from the ambiguity of types.
The idea is great. But then I will ask a counter question. Are you trying to get away from implicit types where?
According to your description, using the ->get('prop') methods is a compromise of maintaining clarity. How long will this kind of clarity be maintained? To put it another way, in Joomla, the parameters use the JRegistry object. Will this object use the ->get() methods forever? You said that there is a plan to abandon magical methods. In this case, it is necessary to switch to the use of magical methods. And after switching to magic methods, in Joomla 5, abandon them altogether, so that in this case, the parameters of the module, plugin, work compatible with the properties. And the transition from methods ->get('prop') to typed properties did not cause pain.
I myself am a very very big proponent of typed properties. In this case, magical methods can help in the transition to typed properties. Without magic methods, it will not be possible to switch from ->get('prop') methods to real properties. That's why I suggested this PR,
get_class($params->prop)
And what is your plan for moving from ->get('prop') methods to typed properties in the future? Just tell everyone what we came up with here: now the JRegistry class will not have get(), set() methods at all.

@nibra
Copy link
Contributor

nibra commented Jul 3, 2022

You can't have typed properties in a Registry, which by design is a bag of arbitrary values of any type. When enforcing usage of $class->get('prop'), you're always aware that you may encounter any data type. With $class->prop you (at least I do) expect a more specific type than mixed. Since you don't know the name of 'prop' beforehand, you can't add type hints or annotations to support IntelliSense.
So, for a class like Registry (or the Container, f.x.), ->get() and ->set() will stay forever, because they're part of the concept of those classes. Nevertheless, there are some places where Registry is used and can be replaced with a proper value object with proper type hinting. Those places will be corrected as we come along.

@korenevskiy
Copy link
Author

@nibra Which IDE editor do you use?

@nibra
Copy link
Contributor

nibra commented Jul 4, 2022

PHPStorm, but that does not really matter. All major IDEs support IntelliSense (although it might have a different name).

@HLeithner
Copy link
Contributor

@korenevskiy you can be sure it wasn't @nibra decision alone to not except this pr as he already stated that it is a production decision.

Beside test if you look at your use case where you want to use JRegistry in the template you get into trouble soon.

echo "<button id='$params[id]>$params[name]";
or
echo "$params->name";

One of the common tasks in a layout is to change the case of a string or do other things before echoing the text.
I created an example at https://3v4l.org/s9DIE#v8.1.7 which uses strtoupper with direct access to the variable. This throws an deprecation warning in php 8+.

You would need to catch the null variable and convert it to a string, doing this in the template looks ugly and counter intuitive and leads to errors.

<?php

class Registry {
    
    public $data = [];
    
    public function __get($var) {
        return $this->data[$var] ?? null;
    }
    
    public function get($var, $default) {
        if (!array_key_exists($var, $this->data)) {
            return $default;
        }
    }
}

$reg = new Registry;

var_dump($reg->empty); // null
var_dump(strtoupper($reg->empty)); // deprecation warning

var_dump($reg->get('empty', 'empty')); // uses the default
var_dump(strtoupper($reg->get('empty', 'empty'))); // uses the default

@korenevskiy
Copy link
Author

korenevskiy commented Jul 4, 2022

@nibra
There is an super extension for PhpStorm for work IntelliSense
https://github.com/klesun/deep-assoc-completion

@HLeithner
You are right, such code is prohibited in PHP 8.
I looked at this code for a long time, almost believed it.
But I realized that there are pitfalls in this code.
logic violation in this code.
Why did you write the second default value in the code? i.e. You intentionally prohibited an error when calling the function, and intentionally caused an error when calling the property.
Your code should be compared. It's fair to compare!

var_dump(strtoupper($reg->empty ?? 'empty')); // NOT deprecation warning
var_dump(strtoupper($reg->get('empty', 'empty'))); // uses the default

In 99% of cases, it is enough for the code to have a default value of NULL. In 1% of cases, a default value is required.

Your example is not proof. Since the use of magical methods in it is either equivalent or also make it more convenient.
$reg->empty??'empty'
Maybe the general production decision was made by mistake due to the fact that the option you described above with substitution of comparison rules was considered? An example of this is to compare boxers from different weight categories. This is a mistake. Either you need to apply default values to both methods, or you don't need to apply it to both!

And now I'll tell you another fact. You are right, text output in layout requires text to be converted into functions. But then it is necessary to specify which text needs to be converted. In the layout, we insert variables: classes, identifiers, attributes to read, labels, Tags eventually.
only for 5%-10% of variable insertion, it is required to use the function of changing the register or removing the buckets.
<field type="moduletag" />
Any fields always have a default value for the default value.
Could the team, when deciding on this PR, have been misled, since the example is unfair for comparison.

The code above is from the number 1%

@nibra
Copy link
Contributor

nibra commented Jul 4, 2022

Could the team, when deciding on this PR, have been misled

No, the decision was made on the basis of architectural considerations, not technical feasibility.

@korenevskiy
Copy link
Author

korenevskiy commented Jul 4, 2022

The extension for PhpStorm that I specified above allows. At any point in the code, set PHPDoc for any variable, as well as set properties and keys in PHPDoc for the same variables. This will allow you to use IntelliSense deep into the variable.
for PHPStorm

/**
 * @var array $arr = [
 *     'fields' => [ // Defines the feilds to be shown by scaffolding
 *         $anyKey => [
 *             'name' => 'sale', // Overrides the field name (default is the array key)
 *             'model' => string, // (optional) Overrides the model if the field is a belongsTo associated value.
 *             'width' => '100px', // Defines the width of the field for paginate views. Examples are "100px" or "auto"
 *             'align' => 'center', // Alignment types for paginate views (left, right, center)
 *             'format' => 'nice', // Formatting options for paginate fields. Options include ('currency','nice','niceShort','timeAgoInWords' or a valid Date() format)
 *             'title' => 'Sale', // Changes the field name shown in views.
 *             'desc' => 'A deal another person that results in money', // The description shown in edit/create views.
 *             'readonly' => false, // True prevents users from changing the value in edit/create forms.
 *             'type' => 'password', // Defines the input type used by the Form helper
 *             'options' => ['option1', 'option2'], // Defines a list of string options for drop down lists.
 *             'editor' => false, // If set to True will show a WYSIWYG editor for this field.
 *             'default' => '', // The default value for create forms.
 *         ],
 *     ],
 * ]
 */
$arr=[
    'fields' => [
        '0' => [
            'model' => 2
        ]
    ]
];

for VSCode

/**
 * @var array $arr $arr
 * @var array $arr['fields']
 * @var array $arr['fields']['fieldName']
 * @var array $arr['fields']['fieldName']['name']
 * @var array $arr['fields']['fieldName']['model']
 * @var array $arr['fields'][fieldName]['width']
 * @var array $arr['fields'][fieldName]['align']
 * @var array $arr['fields'][fieldName]['format']
 * @var array $arr['fields'][fieldName]['title']
 * @var array $arr['fields'][fieldName]['desc']
 * @var array $arr['fields'][fieldName]['readonly']
 * @var array $arr['fields'][fieldName]['type']
 * @var array $arr['fields'][fieldName]['options']
 * @var array $arr['fields'][fieldName]['editor']
 * @var array $arr['fields'][fieldName]['default']
 **/
$arr = [
    'fields' => [
        'fieldName' => []
    ]
];

For JRegistry, a mixed type should not be expected, but a string type should be expected first. Even using the ->get() function, you also need to expect a string value first. Since JRegistry is a serialization and deserialization object. If a person expects a mixed type from JRegistry, then it seems to me that he chose the wrong object. JRegistry has no code to cast one type to another type.

You are right, syntactically restrictions are used in different companies. But each company has its own limitations, they are united by only one common limitation of PSR.
As I wrote above, magic methods remove confusion in writing appeals to the properties of different objects. Confusion may be permissible in understanding the type of object, but I think that such a type can be equated to the stdClass type. This type is formed during diserialization. The confusion is that you can confuse stdClass with Registry, this will cause an error.

@nibra Can you tell us about the architectural limitation?

@nibra
Copy link
Contributor

nibra commented Jul 4, 2022

The extension for PhpStorm that I specified above allows. At any point in the code, set PHPDoc for any variable

You don't need any extension to annotate properties. But you can't set an annotation for a property defined at runtime, as is the case with Registry. If you know the properties beforehand, you should use a value object with proper typing instead.

a string type should be expected first. Even using the ->get() function, you also need to expect a string value first.

Nope. You should always be aware that you know NOTHING about the returned type. That's why magic property access is bad in this case - properties should always have a proper type (different from mixed).

JRegistry is a serialization and deserialization object.

Yes, Registry supports serialisation, but the result will not be what you expect, if objects stored in it do not support serialisation.

Can you tell us about the architectural limitation?

The most obvious thing: We would never be able to add public properties to the Registry. Although there are no plans to do so, good architecture does not create such obstacles.

I really appreciate your effort and I hope that the rejection of this PR does not demotivate you.
If you ever have another suggestion for improvement, let's discuss it first before you put so much work into it.

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.

5 participants