@Fedik interesting approach instead of disabling the separator, still not sure if this would be more understand or better recognize able. But a fix at least for the state class.

Looks nice. Would be cool to somehow set the mode to flat a bit more enhanced than setting the property directly.

Would be cool to somehow set the mode to flat a bit more enhanced than setting the property directly.

It was there #65, but kind of hacky.
As Harald wrote it maybe realy better to have a separated class.
I try to prepare something some time later. But current PR is valid in any case.

I would either also use an argument in the constructor or make a function to flatten it. Like that it looks like an official feature from the registry.

@laoneo are you thinking of something down these lines:

/**
* Set or update the value of the separator property.
*
* @param string $separator The new separator value.
*
* @return void
*/
public function setSeparator(string $separator): void
{
	$this->separator = $separator;
}

/**
* Split a path into an array of keys using the separator property.
*
* If the separator is empty, the path is returned as a single-element array.
*
* @param string $path The path to split.
*
* @return array An array of keys.
*/
public function splitPath(string $path): array
{
	if (empty($this->separator)) {
		return [$path];
	} else {
		return explode($this->separator, $path);
	}
}

With the following tests:

/**
* Test that the separator property can be set and retrieved correctly.
*
* @covers \Joomla\Registry\Registry::setSeparator
*/
public function testSeSeparator()
{
	$registry = new Registry();
	$registry->setSeparator('/');
	$this->assertEquals('/', $this->separator);
}

/**
* Test that a path can be split into an array of keys using the separator property.
*
* @covers \Joomla\Registry\Registry::splitPath
*/
public function testSplitPath()
{
	$registry = new Registry();
	$registry->setSeparator('/');
	$this->assertEquals(['foo', 'bar'], $registry->splitPath('foo/bar'));
}

@nibra what do think about these additions?

The separator should be set using the constructor and be immutable. splitPath() should be private. Thus you don't need to introduce a new SeparatorAware interface.

Hmmm like this?

/**
* Path separator
*
* @var    string
* @since  3.0.0
*/
protected $separator;

/**
* Constructor
*
* @param  mixed   $data       The data to bind to the new Registry object.
* @param  string  $separator  The path separator, and empty string will flatten the registry.
*
* @since   3.0.0
*/
public function __construct($data = null, ?string $separator = null)
{
	// load separator if set
	if (!is_null($separator)) {
		$this->separator = $separator;
	} else {
		$this->separator = '.';
	}

	// Instantiate the internal data object.
	$this->data = new \stdClass();

	// Optionally load supplied data.
	if ($data instanceof self) {
		$this->merge($data);
	} elseif (\is_array($data) || \is_object($data)) {
		$this->bindData($this->data, $data);
	} elseif (!empty($data) && \is_string($data)) {
		$this->loadString($data);
	}
}

So looking at this pull request, where we just want to handle an empty string better... and which basically flattens the registry, This change of making the separator immutable seems to major as it changes the access to the public separator property. What are then the steps forward?

The Registry is broken in this regard. separator should never have been public.
A proper solution would be to define the separator using the constructor, and choose a strategy (either Registry\FlatStrategy or Registry\HierarchicalStrategy) depending on the provided separator. That way you avoid all those "if ($this->separator)" switches. I have no idea ATM for a viable deprecation strategy for the public separator.

I understand your concerns about the public $separator property in the Registry class. However, it's important to remember that this property has been part of the class contract for some time, and changing it now could break code that relies on it. It may not be an ideal design, but it's a reality that we have to deal with and find a solution that works for all parties involved.

As programmers, we should strive to follow best practices and principles of software development, but we also need to be pragmatic and flexible in our approach. While encapsulation is important for maintainability, we should also avoid unnecessary complexity and overhead, especially if it doesn't add significant value or benefits.

Adding new classes to avoid the if..else on the separator value really seems excessive to me. The value can only be a string or null, it could have been a one-liner, but for clarity I wrote a switch.

@Fedik I will merge this PR (after a few more tests), as it fixes an obvious bug/issue, as for the additional fixes I think we will have to target them with separate PRs as our time permits.

However, it's important to remember that this property has been part of the class contract for some time, and changing it now could break code that relies on it. It may not be an ideal design, but it's a reality that we have to deal with and find a solution that works for all parties involved.

Agree. Nevertheless, setting the separator through the constructor can and should be added, as well as a deprecation for the public separator property for 4.0 (for 3.0 it would be too short notice).

Hello Fedik,
thank you for your pull request. To move forward, I would leave this pr as it is and add some more sugar in a followup pr.

I know it's an edge case but what's happen if the separator is 0, using a weak comparison is always wrong ;-)

@Fedik, yes thank you indeed. I actually agree with @laoneo, and yet it should be simple enough to do the following.

Please just add the following to this PR

/**
* Path separator
*
* @var    string
* @deprecated This property will become protected in version 4 to prevent direct access.
* @since   2.0.3
*/
public $separator;

/**
* Constructor
*
* @param  mixed   $data       The data to bind to the new Registry object.
* @param  string  $separator  The path separator, and empty string will flatten the registry.
*
* @since   2.0.3
*/
public function __construct($data = null, string $separator = '.')
{
	// load separator
	$this->separator = $separator;

	// Instantiate the internal data object.
	$this->data = new \stdClass();

	// Optionally load supplied data.
	if ($data instanceof self) {
		$this->merge($data);
	} elseif (\is_array($data) || \is_object($data)) {
		$this->bindData($this->data, $data);
	} elseif (!empty($data) && \is_string($data)) {
		$this->loadString($data);
	}
}

We can improve on this in other PR's but this will set the stage for our transition.

@HLeithner I am not sure about the deprecation notice, and exactly what should be said.

Deprecation message shouldn't be to hard but should be in it's own pr adding the constructor. I also would allow null or false to disable the separator. We would also need a getter for the separator. ( I wouldn't allow to change it afterwards). But I think that discussion is better done in the new PR.

I know it's an edge case but what's happen if the separator is 0, using a weak comparison is always wrong

I also thought about 0, and did not found it much important.
But I can change to check it. I will update later.

It's less about the 0 case it's more about to never use weak comparison. Ymmv

Well the getter and setter functions can solve this. So moving the splitting of the path to its own function and also the setting and getting of the separator will be the best solution.

I agree with @nibra that the function should be private.

We can have multiple PR's I also don't care to much for that, but this change of flattening the registry and dealing with empty separator are kinda related to each other. So I see no problem adding those changes here now, and having that conversation here. unless @Fedik does not want to.

In simple terms we need to catch the empty separator and flatten the path, we want to start down a path that stops the change of the class separator, unless for setting new data, as this change of separator is temporarily based on data path supplied.

For this we want to move all splitting of the path into its own function, and getting and setting of the class separator property.

This should then be easy to mitigate any misunderstanding of what is happening, and why.

@Fedik are you willing to do that? or would you prefer we just make the changes that effectively deals with the empty separator? this is also good with me. We can move these other improvements to its own PR.

Yes please, other pr's, it is also easier to give feedback, instead of having an all in one pr.

I have add better "empty check".

I think it better to do 2 PR, because current one kind of a bug fix and can go in to 2.0.x.
And modifing constructor, kind of a new feature for at least 2.1.x version.

last 2 things, can you add 2 tests one for null and one for 0 (as string or int) please?

Added

@Llewellynvdm I made another PR #68, we can continue discussion there 😉

LGTM