Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.3] Ajax component support of Stringable results #43530

Open
wants to merge 9 commits into
base: 5.3-dev
Choose a base branch
from

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented May 25, 2024

Pull Request for #43471 .

Summary of Changes

Adding support of Stringable https://www.php.net/manual/en/class.stringable.php results to Ajax component.
Adding Joomla\CMS\String\StringableInterface results to Ajax component, this is transitioning interface to Stringable https://www.php.net/manual/en/class.stringable.php.
This allows to customise the response much better, than it is currently.

Testing Instructions

Add following code in to plugins/system/skipto/src/Extension/Skipto.php:

    public function onAjaxSkiptoTest1(\Joomla\CMS\Event\Plugin\AjaxEvent $event)
    {
        $event->updateEventResult(new class ('test 1') extends \Joomla\CMS\Response\JsonResponse implements \Joomla\CMS\String\StringableInterface {});
    }

    public function onAjaxSkiptoTest2(\Joomla\CMS\Event\Plugin\AjaxEvent $event)
    {
        $event->addResult(['foo', 'bar']);
    }

    public function onAjaxSkiptoTestError(\Joomla\CMS\Event\Plugin\AjaxEvent $event)
    {
        throw new \Exception('test error');
    }

Then open following links:

  1. /administrator/index.php?option=com_ajax&plugin=skiptoTest1&group=system&format=raw
  2. /administrator/index.php?option=com_ajax&plugin=skiptoTest1&group=system&format=json
  3. /administrator/index.php?option=com_ajax&plugin=skiptoTest2&group=system&format=json
  4. /administrator/index.php?option=com_ajax&plugin=skiptoTestError&group=system&format=raw
  5. /administrator/index.php?option=com_ajax&plugin=skiptoTestError&group=system&format=json

Expected result AFTER applying this Pull Request

Check response for each:

  1. {"success":true,"message":null,"messages":null,"data":"test 1"}
  2. {"success":true,"message":null,"messages":null,"data":"test 1"}
  3. {"success":true,"message":null,"messages":null,"data":[["foo","bar"]]}
  4. Exception: test error
  5. {"success":false,"message":"test error","messages":null,"data":null}

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:
  • No documentation changes for docs.joomla.org needed
  • Pull Request link for manual.joomla.org: com_ajax Stringable interface Manual#274
  • No documentation changes for manual.joomla.org needed

@korenevskiy
Copy link
Contributor

korenevskiy commented May 29, 2024

Thanks, I tested it. I spent many hours testing on my project.
It can be seen that you spent just as many hours writing, testing, finding bugs and writing documentation for PR.
I tested in JSON format, with error, without error, with data and with a JsonResponse object. I also set the parameters
$input->set('ignoreMessages',false, 'bool'), and also tested in RAW mode, also in different configurations.
I also made an error throw with a message.
Now Joomla has become better. Now we can send customized messages as system messages in case of an error, and special notifications. Respect.

there were no problems during testing.

@Fedik
Copy link
Member Author

Fedik commented May 29, 2024

@korenevskiy Please visit https://issues.joomla.org/tracker/joomla-cms/43530 and push the button, thanks
Screenshot 2024-05-29_23-32-36

@carlitorweb
Copy link
Member

/administrator/index.php?option=com_ajax&plugin=skiptoTest2&group=system&format=raw

Warning: Array to string conversion in [ROOT]\components\com_ajax\ajax.php on line 238

@Fedik
Copy link
Member Author

Fedik commented May 30, 2024

@carlitorweb that is correct, you cannot run skiptoTest2 with format=raw, only with format=json. As in my instruction.
Because format=raw cannot handle anything else than scalar or stringable objects, and skiptoTest2 produces an array.

@carlitorweb
Copy link
Member

I have tested this item ✅ successfully on 66f35b7


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43530.

@korenevskiy
Copy link
Contributor

korenevskiy commented May 30, 2024

I have tested this item ✅ successfully on 66f35b7

there were no problems during testing.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43530.

@Quy
Copy link
Contributor

Quy commented May 30, 2024

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43530.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 30, 2024
@pe7er pe7er assigned pe7er and unassigned pe7er Jun 5, 2024
@rdeutz rdeutz removed the RTC This Pull Request is Ready To Commit label Jun 12, 2024
@rdeutz
Copy link
Contributor

rdeutz commented Jun 12, 2024

We discussed this last week and we think this is a b/c break. It would be better to add a new format instead of changing an existing one and do "kind of magic" on this.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 12, 2024
@Fedik

This comment was marked as outdated.

@korenevskiy
Copy link
Contributor

We discussed this last week and we think this is a b/c break. It would be better to add a new format instead of changing an existing one and do "kind of magic" on this.

By creating a new format, we force each developer to refer to the documentation each time in order to understand the subtleties of the differences between each format. But ultimately the formats are the same, it's just a matter of processing JSON.

@Fedik
Copy link
Member Author

Fedik commented Jun 15, 2024

p


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43530.

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 15, 2024
@Fedik
Copy link
Member Author

Fedik commented Jun 19, 2024

which means backward compatibility is quite simple

Nope, when existing extension provides result as JsonResponse, then it will be wrapped in to another JsonResponse. So in result we will have:
new JsonResponse($anotherJsonResponse) will produce : {"data":{"data":{ ... the object properties}}}.

Even if it does not make sense, it still changing the response that com_ajax will send. and that will break that extension.

@korenevskiy
Copy link
Contributor

new JsonResponse($anotherJsonResponse) will produce : {"data":{"data":{ ... the object properties}}}.

if (!($results instanceof Throwable) && $results instanceof Stringable)

At the same time, it is necessary to add to the \JSONResponsee class
libraries/src/Response/JsonResponse.php

class JsonResponse implements  Stringable{
}

If you do as you say, parents should do this interface for \JSONResponse.

@korenevskiy
Copy link
Contributor

I like your idea, but it seems to me that it is possible to achieve an elegant solution without adding a new interface.

@korenevskiy
Copy link
Contributor

korenevskiy commented Jun 19, 2024

OR

if (!($results instanceof Throwable) && $results instanceof StringableResponseInterface)

At the same time, it is necessary to add to the \JSONResponsee class
libraries/src/Response/JsonResponse.php

class JsonResponse implements  StringableResponseInterface{
}

Where StringableResponseInterface it is joomla new interface

@Fedik
Copy link
Member Author

Fedik commented Jun 19, 2024

No, JsonResponse already implements Stringable implicitly.

Unlike most interfaces, Stringable is implicitly present on any class that has the magic __toString() method defined

And currently we cannot add our StringableInterface to it, because it will change response format. In reasons I wrote earlier.

Currently developers who wants a custom response should implement StringableInterface on their own. And in future (~joomla 7), we change com_ajax to use PHP native Stringable, and all will continue to work without breaks.

@korenevskiy
Copy link
Contributor

korenevskiy commented Jun 19, 2024

You right
Maybe?

if (!($results instanceof Throwable) 
&& ($results instanceof \Joomla\CMS\Response\JsonResponse || \StringableResponseInterface))

Where StringableResponseInterface it is joomla new interface

@korenevskiy
Copy link
Contributor

korenevskiy commented Jun 19, 2024

Is it worth checking the object for JsonSerializable interface?

https://www.php.net/manual/en/jsonserializable.jsonserialize.php

@Fedik
Copy link
Member Author

Fedik commented Jun 19, 2024

That a bit diferent approach, but could work in some cases also.

@korenevskiy
Copy link
Contributor

Why is this method bad?
#43530 (comment)

@Fedik
Copy link
Member Author

Fedik commented Jun 19, 2024

Because as for now it behaviors as "double wrapping", changing it will be b/c break.
#43530 (comment)

@korenevskiy
Copy link
Contributor

korenevskiy commented Jun 19, 2024

if (!($results instanceof Throwable) 
&& ($results instanceof \Joomla\CMS\Response\JsonResponse || \StringableResponseInterface)){
	echo $results;
}else{
	echo new JsonResponse($results, null, false, $input->get('ignoreMessages', true, 'bool'));
}

Where StringableResponseInterface it is joomla new interface

But where is the double explosion here?, because if the object is JsonResponse, then there is no "double wrapping". In this case, the conversion to the string type occurs.

@Fedik
Copy link
Member Author

Fedik commented Jun 19, 2024

because if the object is JsonResponse, then there is no "double wrapping"

"will be".
But current behavior (in j3, j4, j5) is diferent

@korenevskiy
Copy link
Contributor

korenevskiy commented Jun 19, 2024

The previous behavior caused a nested object of class \JsonResponse to be created in a new amendment object of class \JsonResponse , now this object class \JsonResponse will be cast to string .
Do you think there are people who returned the \JsonResponse object from the extension, which was re-serialized in the \JsonResponse object? If there are any, it will cause an error.
Well, then there is no solution.
Because the right decision will cause an error or misunderstanding.
Then let's assign this amendment to Joomla 6. Where it will be explicitly stated that nested \JsonResponse should not be in each other.

@Fedik
Copy link
Member Author

Fedik commented Jun 19, 2024

That what this PR is doing.
You already can use it with #43530 (comment) (when the PR will be merged). Then your code will continue to work in j7, without changes.

@korenevskiy
Copy link
Contributor

I know for sure that the interface name should indicate that the new interface was created to respond to the request.
There will be confusion in assigning the interface name, people will mistakenly inherit from the wrong interface, or for backward compatibility, you will have to store this file for a long time.
StringableResponseInterface

@Fedik
Copy link
Member Author

Fedik commented Jun 23, 2024

The full name Joomla\CMS\String\StringableInterface, so it is fine

@korenevskiy
Copy link
Contributor

What value will this interface have other than the Ajax class of the component?

@exlemor
Copy link

exlemor commented Aug 24, 2024

I have tested this item ✅ successfully on 4ea1349

I have tested this successfully. ( Thanks to @LadySolveig for making sure I didn't kill another test server ;) lol )


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43530.

@HLeithner HLeithner changed the base branch from 5.2-dev to 5.3-dev September 2, 2024 08:51
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.3-dev.

@HLeithner HLeithner changed the title [5.2] Ajax component support of Stringable results [5.3] Ajax component support of Stringable results Sep 2, 2024
@Hackwar Hackwar removed the PR-5.2-dev label Sep 3, 2024
@fgsw
Copy link

fgsw commented Sep 7, 2024

I have tested this item ✅ successfully on 4ea1349


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43530.

@alikon
Copy link
Contributor

alikon commented Sep 7, 2024

rtc


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43530.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature PR-5.3-dev RTC This Pull Request is Ready To Commit
Projects
None yet
Development

Successfully merging this pull request may close these issues.