[#33739] We don't need to require format

[wilsonge]

http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33739&start=0

Raw is already default in the switch/case and therefore isn't needed to be passed in. It also didn't throw an exception on introduction in 3.2 (so technically a b/c break when the exception was introduced)

[betweenbrain]

👍 to simplification

[Bakual]

Maybe it would make sense to have already a default value specified where we define the $format with $input->getWord('format');on line 28? Like this: $input->getWord('format', 'raw');?

[wilsonge]

We could - but does it really matter with the switch having a default case?

[Bakual]

Technically it doesn't matter, and we would still need the default case in the switch statement.

For me it's more a bit better codestyle since you see from the beginning that raw will be the default value.

👍 anyway to this PR 😃

[wilsonge]

Fair enough :) Added it in

[betweenbrain]

The elseif at https://github.com/wilsonge/joomla-cms/blob/patch-20/components/com_ajax/ajax.php#L42 needs to be changed to if. Otherwise, it tests fine.

[wilsonge]

Fixed although I dunno how no one has picked up on this so far :P It's must have been in the component for a while lol

[betweenbrain]

Fixed although I dunno how no one has picked up on this so far :P It's must have been in the component for a while lol

This issue stemmed from the change at wilsonge@3379503. This just illustrates that it's not clearly coded as the removal of the format bit included the initial if statement. I'm honestly not thrilled with the readability, which I can say with a clear conscience since I wrote it. 😆

[betweenbrain]

Thanks for updating the PR @wilsonge. Unfortunately, we have an issue when not specifying a format. Essentially, the entire HTML document is returned with the raw data. One option would be to add $app->close(); at https://github.com/wilsonge/joomla-cms/blob/patch-20/components/com_ajax/ajax.php#L177

@Bakual any thoughts on that change?

[wilsonge]

So that's what we did in 3.2 - so if we don't anymore it's a b/c break

joomla-cms/components/com_ajax/ajax.php

Line 111 in b7ce34a

$app->close();

@piotr-cz was that a mistake in your PR when you did the revamp of com_ajax - or was it intentional (if intentional - why)?

[betweenbrain]

For the record, I was never a fan of requiring the format parameter, but also didn't fight that change.

Regarding b/c, if we can change the component now to not require the format parameter, while still working when it is specified, I wouldn't consider that a b/c issue. But, I suppose you are right that the previous change was a b/c issue, although not likely to affect anyone as it was first included with 3.2.

TL;DR:
I agree

[dbhurley]

I agree with @betweenbrain, I see no need for the requirement on formal parameter. It would not be a b/c issue to drop this requirement as it would not prevent any current implementations from functioning.

👍

[wilsonge]

I kinda meant about the $app->close() thing as well - we shouldn't be pushing out everything - only the module contents I guess?

[betweenbrain]

I kinda meant about the $app->close() thing as well - we shouldn't be pushing out everything - only the module contents I guess?

Agreed.

[Bakual]

any thoughts on that change?

That was probably the reason why format was made required? Especially the &format=raw changes the application flow in Joomla in that it uses a different document renderer.
Closing the app early will prevent execution of certain plugin events. So it's not ideal, however in this case I think closing the app is fine and will not break anything.

The best way would be if we could set the document type to raw during runtime. This used to work in J1.5 but doesn't work anymore since 1.6 I think.
What I did in my own extension is that I check the presence of the format=raw and redirect if it's not set properly. See https://github.com/Bakual/SermonSpeaker/blob/master/com_sermonspeaker/site/controller.php#L37
It's not ideal either but would at least set the correct document type.
One could consider doing something like this.

[piotr-cz]

@wilsonge What do you mean?

[wilsonge]

So in the change here: 94a82ca we removed $app->close() when in raw format. Was there a reason for you removing it or was it a mistake?

[Bakual]

Thinking a bit more about this. I think $format = strtolower($input->getWord('format', 'raw')); should actually default to html. It's the behaviour of Joomla and you can't really change that in a specific component due to already loaded document types.
So the only working way is redirecting to format=raw if no format is given (like I do in my extension) or default to html like Joomla does. Everything else will shoot us in the foot earlier or later.

If we default to html, then the switch statement will actually work correct and we probably don't need the $app->close().

[piotr-cz]

I'm for having the html format as default too to keep the consistency. But it's a b/c change.

As for closing the application, I wouldn't ever do it (so plugins can add other data to response), but there might be a problem as some plugins just assume full html format and alter the output. I'm not sure if it's still happening.

We've had quite a discussion about that in #1960

[Bakual]

I'm for having the html format as default too to keep the consistency. But it's a b/c change.

It shouldn't be a b/c break. Currently if you don't pass the format, you get an error. Afterwards you'll get a HTML response.
We don't change the default in the switch statement.

[piotr-cz]

oh yeah.

[wilsonge]

Tbf the very first version of the plugin defaulted to raw. That was part of what #1960 did - so it could be a b/c change. I dunno

[Bakual]

Tbf the very first version of the plugin defaulted to raw. That was part of what #1960 did - so it could be a b/c change. I dunno

Yeah, and that didn't work, right? And thus we made the format parameter required and throwing and error if not given. So the BC was done then.
Now we don't have a BC if we default to html if no format is given. I would prefer raw or json myself since that is what we likely want most of the time. But due to the way Joomla initialises the document, this doesn't work. At least I'm not aware that we can change the document type during runtime.

[wilsonge]

It did work. #1960 initially was just making the json response use JReponseJson instead of just using json_encode on the results. It scope then got expanded to be a more major refactoring.

[Bakual]

Ah true, the close was there before and got removed and instead the format was made required. If I got that right.
Looks a bit like running in a circle 😃

[betweenbrain]

In my opinion, if we can't default to a more usable format like raw of JSON, I'd just assume leave things as they are then. By returning HTML, when the format parameter is omitted, the page will simply look broken to the end user and no meaningful message will ever get displayed. I'd rather the end user be informed of what is wrong.

I'd prefer that we default to raw, if the format parameter was omitted. 🍶

[betweenbrain]

@piotr-cz

As for closing the application, I wouldn't ever do it (so plugins can add other data to response), but there might be a problem as some plugins just assume full html format and alter the output. I'm not sure if it's still happening.

Do we want other plugins to do this though?

[brianteeman]

Closed as per the comment on the tracker