I have tested this item ✅ successfully on e5ac11b

Thank you!

I have tested this item ✅ successfully on e5ac11b

Good catch

Ready to Commit after two successful tests.

Just wondering what broke that thing as it was working ok since com_fields was released?

The short circuit to divert internal stringToObject calls in the Registry's JSON formatter to the INI formatter was adjusted. It doesn't change the fact though the API has always been documented to require a string parameter, meaning if we had a PHP 7 scalar typed API this scenario would've always failed.

So it is caused by the merge of #20583? Was then #20663 also a product of the bumb?

If you really want to start playing the blame game then yes it was the composer update that broke this in the CMS and it was joomla-framework/registry#43 that exposed these incorrect API uses.

It's not about blaming somebody. It is about to detect if we have a BC break and if we have other parts which do need a fix too.

There is no B/C break. Through a series of fortunate events you basically got lucky that $registry->loadString(null); would end up reaching this block and not raise any kind of error because json_decode() would never be called.

It still doesn't change the fact that the $data parameter of Registry::loadString() and FormatInterface::stringToObject() both document a required string argument and that with scalar typehinting a $registry->loadString(null); or $format->stringToObject(null); call would raise the appropriate errors in a PHP 7 environment.

Inconvenient as the code behavior change is, all it does is expose incorrect use of the API. Our B/C promises do not cover support for undocumented parameter types.

Means now what, we do go through all the code and add checks if it is null or do we make the Registry and it's formats to be able to detect null properly?

Technically it's a B/C break to make Registry deal with nulls.

/*
 * pseudo PHP 7.1 current state
 */

public function loadString(string $data, string $format = 'JSON', array $options = array())

/*
 * pseudo PHP 7.1 supporting nulls
 */

public function loadString(?string $data, string $format = 'JSON', array $options = array())

Making the first param nullable is a signature change and B/C break when you have a strictly typed API. We don't have that luxury because our API has PHP 4 roots and adding types creates B/C breaks, so all we can rely on is doc blocks, whether an argument is required or optional, and when optional what its default value is. In this case, $data is a required argument documented to take a string parameter. string !== null

The code should have ALWAYS been checking for null values before pushing data into one of the string handling functions of the Registry package. The fact the code wasn't doing that and things managed to work just fine before is not indicative of the API supporting null values. It just means you got hella lucky. It does not mean the Registry package's API should be refactored to explicitly support nulls (which as pointed out on a strict typed API is a B/C break anyway because you have to change an existing argument's declaration).

Thats true and I agree in theory. The problem I actually have, is that probably a lot of extensions and I think the core too, do pass null values which have been working before the version bumb.
So the closest I would go is to upgrade the Registry package in 3.9 and not in a patch release. We also need to announce that then clearly.

Revert #20583 then and be done with it. I'm done dealing with this crap.

Superseded by #20678

This is silly. Please.reopen and merge it

Yeah, you're right. I'll let someone with a leadership badge follow through on this silliness instead.

RTC

So it is really not allowed to raise concerns and propose other ways? There is nothing personal going on here in my comments, so please calm down again and focus on the issue we have.

There is no difference in merging the Registry bump in 3.8.9, 3.9.0, or 4.0.0. Regardless of when it lands, it is disruptive if you are using the API wrong and it is a B/C break to adapt the API to the incorrect use of it. So really it is a take it or leave it decision. If you think it is too disruptive for 3.8.9 then it is too disruptive for 3.x.x, period, because if you're going to treat it as disruptive then it might as well be a B/C break.

It was a similar discussion in #19280. When we actually know that there will be potential issues, as this two pr's were revealing it, then all I'm asking for is to do it in a controlled way. And with a minor release we have a bit better chances that actually extension devs are testing it upfront and can act to the change.

#19280 is in no way comparable to this. That one is admitted by all parties to be an API B/C break. This isn't.

Putting it in a minor release doesn't change anything about this. So again, either treat it like a B/C break (which it's not) which effectively locks the Registry package for the rest of the 3.x lifetime or treat it as the inconvenient bug fix it is and that's the end of it.

Apart from NULL. Is this an acceptable behaviour for empty string?

In J3.8.8, the code var_dump((new JRegistry)->loadString('')->toArray()); show empty array.
In the latest staging, the same code raises a PHP Warning.

I quit.

What's the warning? Because if you bother looking in the Registry package repo I very explicitly dealt with the empty string case.

https://3v4l.org/aqapV

PHP is garbage at dealing with JSON decode problems before PHP 7.0.

json_decode() of an empty string or a null value does not work. Our API does not support nulls as documented, I will not entertain any changes to make it work with nulls. As for the empty string condition, if you've got a viable solution for it (should it be confirmed to be an issue) then propose it.

I got this on PHP 7.1.17:

php > (new JRegistry)->loadString('');
PHP Warning:  Uncaught RuntimeException: Error decoding JSON data: Syntax error in .../libraries/vendor/joomla/registry/src/Format/Json.php:72
Stack trace:
#0 .../libraries/vendor/joomla/registry/src/Registry.php(370): Joomla\Registry\Format\Json->stringToObject('', Array)
#1 php shell code(1): Joomla\Registry\Registry->loadString('')
#2 {main}
  thrown in .../libraries/vendor/joomla/registry/src/Format/Json.php on line 72

Warning: Uncaught RuntimeException: Error decoding JSON data: Syntax error in .../libraries/vendor/joomla/registry/src/Format/Json.php:72
Stack trace:
#0 .../libraries/vendor/joomla/registry/src/Registry.php(370): Joomla\Registry\Format\Json->stringToObject('', Array)
#1 php shell code(1): Joomla\Registry\Registry->loadString('')
#2 {main}
  thrown in .../libraries/vendor/joomla/registry/src/Format/Json.php on line 72

Can't be fixed without @ suppression. You're running into another case where the short circuit in the JSON formatter was getting used previously, diverting to the INI formatter, and hitting this block and because the short circuit isn't happening anymore the data is passing forward into json_decode().

You know what, I changed my mind. joomla-framework/registry@182eed3 is the fix to ensure our API will continue to work with developers who refuse to validate their damn data.

Tagged Registry package, updated it here. Now everyone can keep working with their broken code and assumptions. Everyone happy now, or do I need to do something else to appease folks?

(new JRegistry)->loadString(''); or (new JRegistry)->loadString(null);

doesn't mean nothing to me , maybe i'm blind, but this is really a non semantic code
what you should expect to load from null/empty string ????

what you should expect to load from null/empty string ????

People seem to expect it to just work without pointing out the flawed input. But that's fixed now. Nobody has to worry. We can all be happy and go enjoy our rainbows and unicorns and smily emojis.

i'm usually in the wrong side, but at least not now .... so i'm unhappy... i know i'm not in the majority, but this seems insane to me

If we unsure about the input we can always use a constructor.

The correct solution is to require a valid string for JSON format in loadString(...) as Michel did before.
Null and empty string are invalid.

The only problem is that developers quite often put unreliable input into loadString() and would be surprised at 3.8.9.

The only problem is that developers quite often put unreliable input into loadString() and would be surprised at 3.8.9.

these are not developers full stop

The only problem is that developers quite often put unreliable input into loadString() and would be surprised at 3.8.9.

A lot of the Joomla ecosystem follows the core code constructs. The core code is terrible at:

• Handling errors
• Validating data

Because there is 13 years of "m'eh, we don't need to do this" in the core code, there is 13 years of "m'eh, we don't need to do this" in the ecosystem, and every change which changes that to a "what do you mean we have to do this now" is met with anger and frustration because it exposes the broken code. And seemingly the only acceptable fix in Joomla is to allow the broken code to keep working.

Seriously, there is no reason for filesystem errors from Joomla\CMS\Cache\Storage\FileStorage::_deleteFolder() to ever render in the UI. Yet they do.

Seriously, there is no reason for the Registry API to be required to handle null values in an argument which is clearly defined as not supporting them. Yet it does.

I guess we all agree that it is the correct behavior to not accept null. The issue I have is the release we are putting the change into.

It's either a bug fix and goes into 3.8.9 or it's a B/C break and goes into 4.0.0.

There is not a scale for "oh this is a disruptive change so we should put it into a minor release instead of a patch release because people might be more likely to pick up on the disruptive change when testing a new minor release versus testing a patch release".

I do not want to be a party that wants to stop strict requirements because I like the strict (valid data only).
I hate B/C if something does not work as should.

I see, I asked the question in a negative way. It happened that quickly.
I more expected an answer: "Yes, the same as NULL, empty string is invalid data."

In the other side I agree with @laoneo that it should be done in J3.9.

I probably has a problem with understanding B/C rules.

Look, merge this PR. I added b121185, it gets rid of any debate over whether this change is acceptable in a patch release versus minor release, it gets rid of any debate over whether this change is B/C. Any code that might be broken by passing in data that would be considered invalid by the underlying JSON handler will not be potentially broken because I pushed a commit that will do everyone's jobs for them.

There is no B/C break in the Registry changes. Anyone claiming there is is out of their mind. Anyone claiming that a change like that is more likely to be picked up by developers by putting it in 3.9.0 versus 3.8.9 is out of their mind. I don't get why I put together RC packages for testing because they clearly aren't being tested, even by our team who's sole purpose is to test these things (and even when they were provided packages with the security fixes of 3.8.8 with explicit notes on what was changed). So throw the testing argument out the window.

It is a black and white matter. Either the Registry changes are accepted and merged to a patch release, or the CMS Maintainers deem the change to be a B/C break and the Registry package cannot be upgraded again for the 3.x lifetime. It's really that simple. I've been doing release management for a little while, I think I have a grasp on how these things work.

@mbabker your are the release lead so it is up to you.

From a purly practical asspect, this is there for ages, any urgent thing that push us to fix it now and break code from people (put in any more detailed description of the kind of people as your wish).

I would rebase it on 4.0 and document it, job done. We have better things to do.

nevermind