-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Conversation
Co-authored-by: Brian Teeman <[email protected]>
Thanks, I tested it. I spent many hours testing on my project. there were no problems during testing. |
@korenevskiy Please visit https://issues.joomla.org/tracker/joomla-cms/43530 and push the button, thanks |
|
@carlitorweb that is correct, you cannot run |
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. |
I have tested this item ✅ successfully on 66f35b7 there were no problems during testing.
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43530. |
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. |
This comment was marked as outdated.
This comment was marked as outdated.
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. |
p This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43530. |
That a bit diferent approach, but could work in some cases also. |
Why is this method bad? |
Because as for now it behaviors as "double wrapping", changing it will be b/c break. |
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 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. |
"will be". |
The previous behavior caused a nested object of class |
That what this PR is doing. |
I know for sure that the interface name should indicate that the new interface was created to respond to the request. |
The full name |
What value will this interface have other than the Ajax class of the component? |
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. |
This pull request has been automatically rebased to 5.3-dev. |
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. |
rtc This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43530. |
Pull Request for #43471 .
Summary of Changes
Adding support ofStringable
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 toStringable
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
:Then open following links:
/administrator/index.php?option=com_ajax&plugin=skiptoTest1&group=system&format=raw
/administrator/index.php?option=com_ajax&plugin=skiptoTest1&group=system&format=json
/administrator/index.php?option=com_ajax&plugin=skiptoTest2&group=system&format=json
/administrator/index.php?option=com_ajax&plugin=skiptoTestError&group=system&format=raw
/administrator/index.php?option=com_ajax&plugin=skiptoTestError&group=system&format=json
Expected result AFTER applying this Pull Request
Check response for each:
{"success":true,"message":null,"messages":null,"data":"test 1"}
{"success":true,"message":null,"messages":null,"data":"test 1"}
{"success":true,"message":null,"messages":null,"data":[["foo","bar"]]}
Exception: test error
{"success":false,"message":"test error","messages":null,"data":null}
Link to documentations
Please select: