Simplified FlatRegistry class#67
Conversation
@HLeithner I not sure about it, @laoneo wanted to have a CMSObject replacement, that also need a property access. |
|
Using |
|
Instead of using the complex Registry class, I suggest creating a new "mapper" class, like the abstracted mapper which is optimized for flat data structures and is faster. If extended, it would look like the basic path mapper. Although it has a key function, it can be dropped if the goal is simply to store, retrieve or iterate values. Using the Registry class for this task would not be an efficient solution and would result in unnecessary complexity. It's always better to build a more specific solution for a task, rather than relying on a tool that is too powerful. |
|
Okau, sounds valid, I will simplify it more |
I would do it the way to have some deprecated trait as I did here joomla/joomla-cms#39663. The registry should not have direct property access as it is error prone. Especially when you want to have a property with the name data. I liked the approach from #66. If you really want to introduce new Registry implementations (which I think nobody outside of this group will understand why we have them). Then you should really introduce an interface and we change the CMS to that interface. Like that we don't have to care about the implementation. But when something like #66 can already do it, then I would leave the changes as simple as possible. |
|
Okay, I have added Interface, and simplified version. Line 453 in a3932a4 And I not sure how to resolve it correctly. |
|
Question is if we still want to support the separator argument in the future. |
|
In future it probably should be moved to constructor, but for now it should stay for bc. |
I maybe can add set/exists/remove methods to Interface, and commented out |
|
A question. interface RegistryInterface extends \JsonSerializable, \Countable{}Any objections? I think it should be countable by default and also work with json_encode. |
would this create any sideeffect/bypasses any restrictions? I don't think so. so no objections. But what's the target now of this class? extending can reducing |
Yap, an independent lightweight registry. In the same time both |
|
I think it is ready :) |
| * | ||
| * @since 2.1.0 | ||
| */ | ||
| public function loadData($data): RegistryInterface |
There was a problem hiding this comment.
This appears to be the only function that may be causing unexpected issues. I am wondering if we can add more padding here, similar to the approach in https://github.com/joomla-framework/registry/blob/2.0-dev/src/Registry.php#L264 and https://github.com/joomla-framework/registry/blob/2.0-dev/src/Registry.php#L288, in order to set the data type with fewer surprises?
There was a problem hiding this comment.
Originaly I made
public function loadData(iterable $data)But because it does not work with objects I have removed it.
loadArray() and loadObject() kind of the same. But can add it instead of loadData().
There was a problem hiding this comment.
The presence of loadArray, loadObject, and loadFile enhances our control over the flow of data into the local array, ensuring that it is done in a consistent and predictable manner. By maintaining loadData and synthesizing the available options, we can direct the loading process to the most suitable function. This also provides developers with the option of accessing these functions directly if the data type is already known. Ultimately, the goal is to minimize the potential for unexpected edge cases. While there may not be a pressing need to retain loadData, having access to polymorphic functionality can be beneficial in certain scenarios.
| /** | ||
| * Registry Interface | ||
| * | ||
| * @method exists(string $key) |
There was a problem hiding this comment.
I really think these functions need to be written out and not just as comments. So I would deprecate the separator in the 2 branch version and remove it in version 3. So we can start using version 3 then in CMS 5.0. Better to have a clear cut somewhere than doing workarounds and introducing empty interfaces.
There was a problem hiding this comment.
Can be, but I thought it can go to 2.x.
There was a problem hiding this comment.
I am a strong advocate for preserving tradition while embracing progress. This means that I try to minimize breaking changes and avoid them whenever possible. However, in this instance, adding the shared public functions from both classes to the interface will not cause any disruptive changes. As a result, I believe it is appropriate to proceed with this update.
We need to be cautious about implementing return type declarations and function argument data types until we are fully prepared to transition to version 3, so the current Registry class must dictate the interface.
Regarding the deprecation of the separator in the 2 branch version, I'm not sure what you mean by this. @laoneo, could you clarify your intentions regarding the usage of the separator in the current Registry class? or are you speaking about the new flat class?
There was a problem hiding this comment.
Check the discussion above about the set function: I would love to see an interface like the following code (is not complete as I didn't add all the functions from the doc block:
interface RegistryInterface extends \JsonSerializable, \Countable
{
public function exists($path);
public function get($path, $default = null);
public function set($path, $value);
}
There was a problem hiding this comment.
That's why the interface needs to go into 3.0 where we remove the separator argument from the registry class.
There was a problem hiding this comment.
The need to set the separator at the call level has been useful in many cases, specially with setting new data. Hmm maybe we are mixing 2 things that should not be mixed.
|
coming back to the use case for this object. what should it be used for? Then the question is why shouldn't be final? based on KISS |
|
Use case: A Flat-Registry (or Fast Registry) that, as far as I understand, should be capable of replacing CMSObject. Wasn't this pull request started with that goal in mind? I agree with the KISS principle to avoid scope creep. Shared interfaces offer so many more use cases, which is always a win. However, I believe that the main objective, correct me if I'm wrong, is to create a simplified registry that disregards |
|
For my understanding the CMSObject is a base class for rich objects in opposite to an registry which is only a storage object. I would like to be clear first what we want and need then then maybe find a better name if it doesn't fit. |
|
Honestly I'm in favor of #66 as you can read in my comment above. I was just mentioning, when we introduce multiple implementations of a registry, then we need an interface. So I would just do #66 with a proper function to reset the separator (not only the constructor) and all is good. I don't see a need for a flat or fast or tiny registry implementation. |
I thought about it as Flat-Registry (or Fast Registry), a very basic storage, compatible with big brother Registry. If you guys think it not really need, I can close it, before it become another complicated registry 😉 |
|
I'm also open for a stupid simple registry which only holds a bag and is not extendable. |
|
Why make it non-extendable? To me, the whole point of creating a simple registry is to provide a foundation for other, more practical, yet still straightforward classes with slightly more options. In reality, what Joomla framework doesn't do, I can easily do outside of it. So adding this class would be great, but it would only make sense to me if it were extendable. While I'm using the containerized approach, building a small, new registry-like class is still a powerful design choice. Looking at these implementation by third party extensions: If we had a flat registry, I could replace these with Joomla's new class: Well, as I said, I don't need Joomla to agree with me, but freedom is just as important as good design. |
|
I see different use cases for this, if you have something that can be extended you automatically could/will break things on updates. if you have final classes for a specific task you can do what you want when in the end the api stays the same. We can do all 3 variants no problem with it. But thinking of the future and which class should be used for which usecase is important. The framework or better the Software Architecture & Strategy Team should answer such questions and implement it (if possible) |
|
As a side note, there are very few final classes in the framework - only about 30 out of approximately 1,000 classes. |
|
Just to be clear that was no offence against SAST or you. Just a badly formed motivation to get some more people involved to SAST |
|
Since the class is backed up by an interface, it should be final. Inheritance is not really a good thing. |
|
hmhm I thought a bit, it seems not much use case for this class. I had in mind to try it as registry for joomla/joomla-cms#39657 for CaptchaRegistry.php (and for couple PR that will follow), but it require extending to allow the value type validation, and it seems more easy to have a separated class for this case. I closing it for now, can redo when we will be need something like this. |
Instead of inheritance, you should design it as a decorator. That way, it can be applied to any class implementing the interface. |
Summary of Changes
This is kind of alternative for #65
A simplified registry, which ignores $separator at all.
What you guys think about it?