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

fix(menu/save-project): Add missing XML prolog on document save #1173

Merged
merged 5 commits into from
Feb 27, 2023

Conversation

danyill
Copy link
Collaborator

@danyill danyill commented Feb 14, 2023

Most of the DOM methods for detecting the declaration/prolog are deprecated (Document.xmlVersion, Document.xmlEncoding, DOMImplementation.hasFeature()) so I avoided using them. There do not appear to be replacements.

As per previous discussions on Save Project, I don't think we can add a test without modifying the code for the test.

I think we should leave the issue open until we have a test for this as part of the Action/Editing API. i.e. carry out action, check that the document is well-formed.

I would be grateful for testing/review as this is quite important.

Part of #1163

@danyill danyill requested a review from ca-d February 14, 2023 20:36
@danyill danyill force-pushed the issue-1163-missing-xml-prolog branch from daae9ec to 496a6ab Compare February 14, 2023 20:58
@danyill danyill changed the title Add missing XML prolog on document save fix(menu/save-project): Add missing XML prolog on document save Feb 15, 2023
ca-d
ca-d previously approved these changes Feb 20, 2023
Copy link
Contributor

@ca-d ca-d left a comment

Choose a reason for hiding this comment

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

LGTM, but is there a chance this might lead to prologue duplication in case there's a prologue with whitespace nodes before it or something (e.g. attribute order) is formatted differently? I don't know if these things can happen, or if there could ever be other encoding attribute values e.g.

src/menu/SaveProject.ts Outdated Show resolved Hide resolved
@danyill
Copy link
Collaborator Author

danyill commented Feb 20, 2023

LGTM, but is there a chance this might lead to prologue duplication in case there's a prologue with whitespace nodes before it or something (e.g. attribute order) is formatted differently? I don't know if these things can happen, or if there could ever be other encoding attribute values e.g.

I believe at best the document which did this would not be well-formed which is a reasonable requirement (?).

The spec requires that to be well formed the prolog should be unambiguously the first item (after the byte order mark): https://www.w3.org/TR/xml/#sec-prolog-dtd see section 2.1

@danyill danyill requested a review from ca-d February 20, 2023 19:44
@danyill
Copy link
Collaborator Author

danyill commented Feb 26, 2023

Incidentally, it amuses me that we didn't notice this earlier. Anyone with a file editor GUI which inspected the files might have shown something like this:

image

In due course, I'll do a separate PR to tidy these and likely others.

Copy link
Contributor

@ca-d ca-d left a comment

Choose a reason for hiding this comment

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

Sorry for not re-approving that sooner, LGTM.

@danyill danyill merged commit 6cae0da into openscd:main Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants