Skip to content

Rename XmlParser to XMLParser for consistency with XMLWriter/XMLReader #6451

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

Closed
wants to merge 1 commit into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Nov 24, 2020

@sgolemon @carusogabriel as RMs for PHP 8

@carusogabriel
Copy link
Contributor

carusogabriel commented Nov 24, 2020

Hm, this is a tricky one. While I agree, today is GA day. Merging it into 8.0.1 is a BC-break with 8.0.0, and merging it into 8.0.0 is a BC-break with RC5, which is a problem because RC5 and GA should match.

I'll deffer this decision to Sara :)

@Girgias
Copy link
Member Author

Girgias commented Nov 24, 2020

Well it's technically not BC as Class names as case insensitive in PHP... so not sure if it really matters in the end.

@carusogabriel
Copy link
Contributor

It does, unfortunately 😕

Sniffers and Static Analyzers, for example, they would complain that you are using/importing XMLParser with the wrong case.

@GinoPane
Copy link

GinoPane commented Nov 25, 2020

But XmlParser is a correct camel-cased name. It would be better to rename XMLWriter/XMLReader to XmlWriter/XmlReader instead

@claudepache
Copy link
Contributor

claudepache commented Nov 27, 2020

But XmlParser is a correct camel-cased name. It would be better to rename XMLWriter/XMLReader to XmlWriter/XmlReader instead

It is good to look at precedents. In general, both conventions are in use (JsonException vs.DOMException). But in the case of XML, the pattern is clear:

LibXMLError
SimpleXMLElement
SimpleXMLIterator
XmlParser // <-- the outsider, just introduced in 8.0
XMLReader
XMLWriter

Note also:

XSLTProcessor

and:

DOMException
DOMImplementation
DOMNode
DOMNameSpaceNode
DOMDocumentFragment
DOMDocument
DOMNodeList
DOMNamedNodeMap
DOMCharacterData
DOMAttr
DOMElement
DOMText
DOMComment
DOMCdataSection
DOMDocumentType
DOMNotation
DOMEntity
DOMEntityReference
DOMProcessingInstruction
DOMXPath

@GinoPane
Copy link

But XmlParser is a correct camel-cased name. It would be better to rename XMLWriter/XMLReader to XmlWriter/XmlReader instead

It is good to look at precedents. In general, both conventions are in use (JsonException vs.DOMException). But in the case of XML, the pattern is clear:

LibXMLError
SimpleXMLElement
SimpleXMLIterator
XmlParser // <-- the outsider, just introduced in 8.0
XMLReader
XMLWriter

Note also:

XSLTProcessor

and:

DOMException
DOMImplementation
DOMNode
DOMNameSpaceNode
DOMDocumentFragment
DOMDocument
DOMNodeList
DOMNamedNodeMap
DOMCharacterData
DOMAttr
DOMElement
DOMText
DOMComment
DOMCdataSection
DOMDocumentType
DOMNotation
DOMEntity
DOMEntityReference
DOMProcessingInstruction
DOMXPath

Yes, I understand, it is true in terms of what is widely used in the current codebase. But if we speak about camel-case in general, all those accepted names turn to be not really correct. It does not mean we should fix them of course

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Seems worthwhile to fix, given how consistently the XML capitalization is used right now.

I think this is okay for 8.0.1 as class names are case-insensitive, so at worst this should affect some code style checking.

@Girgias
Copy link
Member Author

Girgias commented Nov 30, 2020

Merged as a55402d

@Girgias Girgias closed this Nov 30, 2020
@Girgias Girgias deleted the rename-xmlparser branch November 30, 2020 14:10
@carusogabriel carusogabriel added this to the PHP 8.0 milestone Nov 30, 2020
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.

5 participants