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

Allow stdClass in XML responses #38745

Merged
merged 1 commit into from
Jun 13, 2023
Merged

Conversation

provokateurin
Copy link
Member

Summary

Allows returning \stdClass() in DataResponse that are encoded to XML.
JSON can already to it and converts it to {}, but for XML special handling is required.

This change is required in order to properly describe APIs using empty JSON objects ([] gets converted to [] in JSON instead of {}). XML just also needs to support it.

Checklist

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be tested in \Test\AppFramework\Middleware\BaseResponseTest?

@provokateurin provokateurin force-pushed the feature/ocs-xml-stdclass branch 2 times, most recently from 2a75103 to 587ad1b Compare June 12, 2023 15:03
Signed-off-by: jld3103 <jld3103yt@gmail.com>
@provokateurin provokateurin merged commit 38d64f4 into master Jun 13, 2023
@provokateurin provokateurin deleted the feature/ocs-xml-stdclass branch June 13, 2023 11:30
Jenandra pushed a commit to Jenandra/nextcloud-server that referenced this pull request Jun 13, 2023
…class

Allow stdClass in XML responses

Signed-off-by: Silke Suck <silke.suck@adacor.com>
@AlexM4H
Copy link

AlexM4H commented Aug 31, 2023

Many thanks for the PR. A backport to Nextcloud 27.1.0 would be desirable.

@provokateurin
Copy link
Member Author

@AlexM4H does it fix a bug for you? This was only meant as a feature, so it doesn't get any backport

@AlexM4H
Copy link

AlexM4H commented Aug 31, 2023

It fixes #1469 and #33330 for me.

Otherwise /api/v2.1/form/{id} returns an empty XML result.

@provokateurin
Copy link
Member Author

On a technical level this is still a bug with the respective apps and not the core as this was never supported. @ChristophWurst @come-nc shall we backport it anyway?

@come-nc
Copy link
Contributor

come-nc commented Aug 31, 2023

Hard to say. Backporting API change is always dangerous as it may trigger subtle sideeffects.
On the other hand if it fixes bugs…

Without this an stdClass is just stripped from XML output?

@provokateurin I think you’re more knowledgeable than me to decide whether to backport.

@provokateurin
Copy link
Member Author

provokateurin commented Aug 31, 2023

It's not breaking at all, but if an app returns stdClass the output without this patch would just be empty thus broken.

I still feel like the apps should fix it themselves as any version before this patch is broken. That would mean apps can't target <28 if they rely on this.

@AlexM4H
Copy link

AlexM4H commented Aug 31, 2023

Please keep in mind that Forms is a featured app. "Featured apps are developed by and in the community. They provide core functionality and are ready for production use."

The fix would be necessary for some app developers for NC 26 / 27, as NC 28 already includes the fix as a new feature.

@Chartman123
Copy link

What would we need to do to fix the problem in Forms?

@AlexM4H
Copy link

AlexM4H commented Aug 31, 2023

The corresponding issue is still open

nextcloud/forms#1469

@Chartman123
Copy link

@AlexM4H yes, I know. We're waiting for a server fix there 😆

My question was for @provokateurin 😉

@provokateurin
Copy link
Member Author

provokateurin commented Aug 31, 2023

Please keep in mind that Forms is a featured app. "Featured apps are developed by and in the community. They provide core functionality and are ready for production use."

@AlexM4H Sorry, this is not helpful. This is a bug with the app and not the core.

As I said the behavior the forms app is relying on was never intended to work (in previous versions). I added this as a feature but it is only available in >=28. If you target 28 and higher you can surely rely on it.

What would we need to do to fix the problem in Forms?

@Chartman123 For example Talk also had to work around it like this: https://github.com/nextcloud/spreed/blob/eaf04bbe5a7626cc1565cc455124f8a10b2ff078/lib/Model/Message.php#L163

cc @DaphneMuller

@Chartman123
Copy link

For example Talk also had to work around it like this

Thanks I'll have a look at it :)

@provokateurin
Copy link
Member Author

In the future when you target >=28 you can of course drop the workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants