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

CDATA nodes' content can only be retrieved using XMLParser.get_node_name(). #89120

Closed
elenakrittik opened this issue Mar 3, 2024 · 1 comment · Fixed by #89141
Closed

CDATA nodes' content can only be retrieved using XMLParser.get_node_name(). #89120

elenakrittik opened this issue Mar 3, 2024 · 1 comment · Fixed by #89141

Comments

@elenakrittik
Copy link
Contributor

Tested versions

Reproducible on 4.2.1 (and it seems ever since 3.5 at least, #65416 )

System information

Godot v4.2.1.stable - Android - GLES3 (Compatibility) - Mali-G52 - (8 Threads)

Issue description

From #65416:

The CDATA content can be read via get_node_name() (wich is odd .. imho).

get_node_name() currently says

This method will raise an error if the currently parsed node is not of NODE_ELEMENT or NODE_ELEMENT_END type.

But in practice does not for NODE_CDATA nodes and instead returns CDATA content. As it seems like fair amount of people already rely on this, (aside from correcting documentation, of course) i propose to soft-deprecate the current behaviour and instead make get_node_data() responsible for returning CDATA nodes' content.

Steps to reproduce

Download the test project and run it. It will print the following (excluding the arrow note):

Node name: data
   At: res://main.gd:12:_ready()
Is node a CDATA node: false
   At: res://main.gd:13:_ready()
Node name: 
   At: res://main.gd:12:_ready()
Is node a CDATA node: false
   At: res://main.gd:13:_ready()
Node name:  Hello World          <-- here
   At: res://main.gd:12:_ready()
Is node a CDATA node: true
   At: res://main.gd:13:_ready()
Node name: data
   At: res://main.gd:12:_ready()
Is node a CDATA node: false
   At: res://main.gd:13:_ready()

Minimal reproduction project (MRP)

xmlcdata.zip

@timothyqiu
Copy link
Member

Thanks for the report!

The documentation for XMLParser is written way after the implementation. So I believe it's get_node_name()'s description that's wrong. This method errors when the node is a text node, exactly opposite of get_node_data().

The name (as well as the API) is indeed confusing. But on the other side, changing the behavior breaks compatibility with existing projects. Personally, I don't think bad naming is a good reason for compat breaks :P

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

Successfully merging a pull request may close this issue.

3 participants