Skip to content

Conversation

@hartwork
Copy link
Contributor

@hartwork hartwork commented Sep 30, 2025

Clarify that:
- it takes parsing for an attack
- that some doors are closed by default
- only version 2.7.2 has all the fixes
- use of the bundle depends on configuration
@hartwork
Copy link
Contributor Author

hartwork commented Oct 4, 2025

@vstinner I saw your related work in commit cb99d99. Would you be up for review here?

@hartwork
Copy link
Contributor Author

@picnixz I was hoping for this PR to be a quick and uncontroversial win for everyone. Would you be up to get this over the finish line with me? Else could you connect me to someone up for it? I believe @hedsnz uncovered some actual issues in #139313 that are worth addressing.

The keys ideas in this pull request are:

  • warn about ExternalEntityRefHandler right where users will be looking
  • try to make clear that XML issues are not a problem of Python in particular
  • that the defaults are pretty good by now
  • that >=2.6.0 is no longer recent enough but >=2.7.2
  • that use of system Expat depends on configuration while resisting to copy details but refer to another page
  • link to upstream Expat for users unknown to Expat

What do you think?

@picnixz picnixz self-requested a review October 13, 2025 17:53
@hartwork hartwork requested a review from vstinner October 14, 2025 13:29
Copy link

@hedsnz hedsnz left a comment

Choose a reason for hiding this comment

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

Thanks very much @hartwork, this PR addresses everything in the issue I originally raised, and I think it strikes a good balance between giving enough background so that people know where to start looking for further information, while not giving so much information that is better documented elsewhere.

I've offered a couple of very minor wording suggestions.

Original idea by @hedsnz with adjustments
As suggested by @vstinner
@hartwork
Copy link
Contributor Author

hartwork commented Oct 20, 2025

Thanks very much @hartwork, this PR addresses everything in the issue I originally raised, and I think it strikes a good balance between giving enough background so that people know where to start looking for further information, while not giving so much information that is better documented elsewhere.

@hedsnz cool, thanks for your support!

I've offered a couple of very minor wording suggestions.

Thanks for the review!

@hartwork
Copy link
Contributor Author

@vstinner could this be merged please please?

@StanFromIreland
Copy link
Member

Note that the general guideline around here is to wait a month before pinging, there is a long queue of PRs (>2000!).

@hartwork
Copy link
Contributor Author

hartwork commented Oct 24, 2025

@StanFromIreland I understand that the project has many other pull request but I'll be honest with you, I only jumped in to help with this PR here (that was not my own pain point) because I felt like it could be a quick win, and this pull request is way too trivial and tiny to have a life of 3 weeks in my book. It only means that I will twice to be the guy providing doc fixes like this next time. I'm not sure that's intended. I'm happy to team up with you personally on other pull request review in CPython as feasible if that helps you move faster on the queue. Thanks for considering my side.

@vstinner
Copy link
Member

@vstinner could this be merged please please?

I don't know XML very well. Maybe @encukou or @serhiy-storchaka would like to review it.

The previous version was apparantly not clear enough.
@hartwork
Copy link
Contributor Author

I don't know XML very well. Maybe @encukou or @serhiy-storchaka would like to review it.

@vstinner thanks!

(I was asking you because of your commit cb99d99 on Doc/library/xml.rst.)

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. Should we backport this change to 3.13 and 3.14?

@hartwork
Copy link
Contributor Author

LGTM.

@vstinner thank you!

Should we backport this change to 3.13 and 3.14?

I have a feeling the question is probably more towards other Python core devs. If you decide for it and @bedevere-bot has trouble auto-cherry-picking, I'll be happy to help with a manual backbort.

@serhiy-storchaka serhiy-storchaka added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Oct 29, 2025
@serhiy-storchaka
Copy link
Member

If it is worth to be committed in main, it should be backported.

@hartwork
Copy link
Contributor Author

hartwork commented Nov 5, 2025

Ready to merge?

@serhiy-storchaka serhiy-storchaka merged commit baa9f33 into python:main Nov 5, 2025
32 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in Docs PRs Nov 5, 2025
@miss-islington-app
Copy link

Thanks @hartwork for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 5, 2025
Clarify that:
- it takes parsing for an attack
- that some doors are closed by default
- only Expat version 2.7.2 has all the fixes
- use of the bundle depends on configuration
(cherry picked from commit baa9f33)

Co-authored-by: Sebastian Pipping <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Nov 5, 2025

GH-141065 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Nov 5, 2025
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 5, 2025
Clarify that:
- it takes parsing for an attack
- that some doors are closed by default
- only Expat version 2.7.2 has all the fixes
- use of the bundle depends on configuration
(cherry picked from commit baa9f33)

Co-authored-by: Sebastian Pipping <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Nov 5, 2025

GH-141066 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Nov 5, 2025
serhiy-storchaka pushed a commit that referenced this pull request Nov 5, 2025
Clarify that:
- it takes parsing for an attack
- that some doors are closed by default
- only Expat version 2.7.2 has all the fixes
- use of the bundle depends on configuration
(cherry picked from commit baa9f33)

Co-authored-by: Sebastian Pipping <[email protected]>
serhiy-storchaka pushed a commit that referenced this pull request Nov 5, 2025
Clarify that:
- it takes parsing for an attack
- that some doors are closed by default
- only Expat version 2.7.2 has all the fixes
- use of the bundle depends on configuration
(cherry picked from commit baa9f33)

Co-authored-by: Sebastian Pipping <[email protected]>
@hartwork
Copy link
Contributor Author

hartwork commented Nov 5, 2025

@serhiy-storchaka thank you! 🙏

@serhiy-storchaka
Copy link
Member

Thank you for your contribution.

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

Labels

docs Documentation in the Doc dir skip news

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants