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

String.format depends on the order of passed values, resulting in unwanted replacements. #89604

Closed
elenakrittik opened this issue Mar 17, 2024 · 2 comments · Fixed by #89608
Closed

Comments

@elenakrittik
Copy link
Contributor

Tested versions

Reproducible in 4.2.1.stable and v4.3.dev.custom_build [fe01776] (freshly synced and built master)

System information

Godot v4.3.dev (fe01776) - Windows 10.0.19045 - Vulkan (Mobile) - dedicated NVIDIA GeForce RTX 3060 (NVIDIA; 31.0.15.5123) - AMD Ryzen 7 5800X 8-Core Processor (8 Threads)

Issue description

If you have a "template" string "{first}{second}", the result of .format()ting it will depend on the order of values passed in. Specifically, if the passed value for {first} is a string that contains literal "{second}", that literal will get replaced with the value of {second} if the value for {first} was passed before the value for {second}.

Steps to reproduce

Create a new project and assign the following script to the main scene.

extends Node

func _ready() -> void:
    var template := "{first}{second}"

    var first_value := "{second}"
    var second_value := "Hello!"

    var formatted := template.format({
        # if we swap the next two lines, `second` will get formatted before `first`,
        # and the unwanted replacement will not occur
        "first": first_value,
        "second": second_value,
    })

    print(formatted) # "Hello!Hello!"

Minimal reproduction project (MRP)

N/A

@AThousandShips
Copy link
Member

AThousandShips commented Mar 17, 2024

This is a bit of a complex situation, "fixing" this would risk breaking existing code so it might be best to document this instead, but depends on how serious it's considered

Reading through the original PR it seems this is not an oversight, and I believe this should be documented clearly instead, as it is a useful feature, as long as you're aware of it

@Kiisu-Master
Copy link
Contributor

This is a bit of a complex situation, "fixing" this would risk breaking existing code so it might be best to document this instead, but depends on how serious it's considered

Reading through the original PR it seems this is not an oversight, and I believe this should be documented clearly instead, as it is a useful feature, as long as you're aware of it

I don't think documenting this makes a lot of sense, because what it's doing is equivalent to 2 lines of GDSript and it would be nice if it had more functionality:

for key in values:
    template = template.replace("{" + key + "}", values[key])

This behavior of replacing the keys in already replaced parts of the string can be very problematic when dealing with user input.

This can for example corrupt data in the XML tool by elenakrittik. When setting an XML node attribute value to the string "{children}", it will replace that with the actual string of the XML nodes children when saving the data, because it uses format strings like "<{node_name}{attributes}>{content}{cdata}{children}</{node_name}>".

I also tested pythons string format function to have something to compare with. It formats non-recursively by default.

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