8000 Use new PHPCS array syntax by carusogabriel · Pull Request #84 · doctrine/coding-standard · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use new PHPCS array syntax #84

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 24, 2018
Merged

Use new PHPCS array syntax #84

merged 1 commit into from
Sep 24, 2018

Conversation

carusogabriel
Copy link
Contributor

As described in the upgrade guide of PHPCS v3.3.0, there's a new array syntax in favor of the old one that was deprecated and will be removed in v4. Here's an example:

-<property name="forbiddenFunctions" type="array" value="sizeof=>count,delete=>unset,print=>echo,is_null=>null,create_function=>null"/>
+<property name="forbiddenFunctions" type="array">
+    <element key="sizeof" value="count"/>
+    <element key="delete" value="unset"/>
+    <element key="print" value="echo"/>
+    <element key="is_null" value="null"/>
+    <element key="create_function" value="null"/>
+</property>

@carusogabriel carusogabriel added this to the 5.0.0 milestone Sep 2, 2018
@carusogabriel carusogabriel requested a review from a team as a code owner September 2, 2018 20:15
@Majkl578
Copy link
Contributor
Majkl578 commented Sep 2, 2018

cc @kukulich, is this ok and properly compatible with the SettingsHelper (thus Slevomat CS sniffs)?

kukulich
kukulich reviewed Sep 3, 2018
View reviewed changes
lib/Doctrine/ruleset.xml
<property name="traversableTypeHints" type="array">
<element value="Doctrine\Common\Collections\Collection"/>
</property>
<property name="usefulAnnotations" type="array">
Copy link
Contributor

Choose a reason for hiding this comment

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

@carusogabriel Btw you can use allAnnotationsAreUseful now, see https://github.com/slevomat/coding-standard#slevomatcodingstandardtypehintstypehintdeclaration-

Copy link
Contributor

Choose a reason for hiding this comment

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

That's probably for a separate discussion. But given that we already use forbiddenAnnotations, we may consider this change and just expand the forbidden list for any of those that get removed by this rule.

 8000

@kukulich
Copy link
Contributor
kukulich commented Sep 3, 2018

Yes, it should be compatible.

alcaeus
alcaeus previously approved these changes Sep 3, 2018
View reviewed changes
Majkl578
Majkl578 suggested changes Sep 3, 2018
View reviewed changes
Copy link
Contributor
@Majkl578 Majkl578 left a comment

Choose a reason for hiding this comment

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

This is a move in good direction, but PHPCS guys forgot to update their XML schema. So this PR now results in invalid XML according to vendor/squizlabs/php_codesniffer/phpcs.xsd. :(
Reported here: squizlabs/PHP_CodeSniffer#2151

@Majkl578
Copy link
Contributor
Majkl578 commented Sep 3, 2018 •
edited
Loading

xmllint report:

$ xmllint --noout --schema vendor/squizlabs/php_codesniffer/phpcs.xsd lib/Doctrine/ruleset.xml 
lib/Doctrine/ruleset.xml:53: element property: Schemas validity error : Element 'property': The attribute 'value' is required but missing.
lib/Doctrine/ruleset.xml:53: element property: Schemas validity error : Element 'property': Character content is not allowed, because the content type is empty.
lib/Doctrine/ruleset.xml:53: element property: Schemas validity error : Element 'property': Element content is not allowed, because the content type is empty.
lib/Doctrine/ruleset.xml:129: element property: Schemas validity error : Element 'property': The attribute 'value' is required but missing.
lib/Doctrine/ruleset.xml:129: element property: Schemas validity error : Element 'property': Character content is not allowed, because the content type is empty.
lib/Doctrine/ruleset.xml:129: element property: Schemas validity error : Element 'property': Element content is not allowed, because the content type is empty.
lib/Doctrine/ruleset.xml:148: element property: Schemas validity error : Element 'property': The attribute 'value' is required but missing.
lib/Doctrine/ruleset.xml:148: element property: Schemas validity error : Element 'property': Character content is not allowed, because the content type is empty.
lib/Doctrine/ruleset.xml:148: element property: Schemas validity error : Element 'property': Element content is not allowed, because the content type is empty.
lib/Doctrine/ruleset.xml:268: element property: Schemas validity error : Element 'property': The attribute 'value' is required but missing.
lib/Doctrine/ruleset.xml:268: element property: Schemas validity error : Element 'property': Character content is not allowed, because the content type is empty.
lib/Doctrine/ruleset.xml:268: element property: Schemas validity error : Element 'property': Element content is not allowed, because the content type is empty.
lib/Doctrine/ruleset.xml:271: element property: Schemas validity error : Element 'property': The attribute 'value' is required but missing.
lib/Doctrine/ruleset.xml:271: element property: Schemas validity error : Element 'property': Character content is not allowed, because the content type is empty.
lib/Doctrine/ruleset.xml:271: element property: Schemas validity error : Element 'property': Element content is not allowed, because the content type is empty.
lib/Doctrine/ruleset.xml fails to validate

@Majkl578 Majkl578 mentioned this pull request Sep 3, 2018
Closed
@Ocramius
Copy link
Member
Ocramius commented Sep 3, 2018

Rebase required

@Majkl578
Copy link
Contributor
Majkl578 commented Sep 4, 2018 •
edited
Loading

Waiting for PHPCS 3.3.2 with a fix: squizlabs/PHP_CodeSniffer@cc5c930

@carusogabriel carusogabriel modified the milestones: 5.0.0, 6.0.0 Sep 6, 2018
@Majkl578
Copy link
Contributor
Majkl578 commented Sep 9, 2018

I think this would be a good stuff for 5.0.0, but I don't want to wait for PHPCS 3.2.2 with unknown release date.
We can ship this regardess, just the XML would be invalid according to XML schema, until PHPCS 3.2.2 is shipped. Thoughts @Ocramius @alcaeus @doctrine/team-coding-standards?

@carusogabriel
Copy link
Contributor Author

I'd wait for the new PHPCS release, as we've already marked this one in the 6.0v.

@Majkl578
Copy link
Contributor
Majkl578 commented Sep 9, 2018

as we've already marked this one in the 6.0v

Well, you did, I already mentioned this issue and getting it into 5.0.0 earlier this week. :)
But it shouldn't matter anyway, consumers may use the new syntax regardless.

@deeky666
Copy link
Member

is the "rush" worth the benefit? what do we get by releasing something "half-valid" before 6.0?

@Majkl578
Copy link
Contributor

Let's see what happens sooner: us releasing 5.0.0 or PHPCS releasing 3.3.2 (may happen this week). :)

@Majkl578 Majkl578 mentioned this pull request Sep 20, 2018
Merged
@Majkl578
Copy link
Contributor

3.3.2 was just released, can you please rebase + require 3.3.2? Will merge then, thanks.

@Majkl578 Majkl578 modified the milestones: 6.0.0, 5.0.0 Sep 23, 2018
@carusogabriel carusogabriel requested a review from a team September 24, 2018 11:02
Ocramius
Ocramius approved these changes Sep 24, 2018
View reviewed changes
Copy link
Member
@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

🚢

@Ocramius Ocramius merged commit 2c78365 into master Sep 24, 2018
@Ocramius Ocramius deleted the chore/new-arry-format branch September 24, 2018 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kukulich kukulich kukulich left review comments

@Majkl578 Majkl578 Majkl578 approved these changes

@Ocramius Ocramius Ocramius approved these changes

@alcaeus alcaeus alcaeus approved these changes

@greg0ire greg0ire greg0ire approved these changes

Assignees
No one assigned
Labels
Improvement
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
7 participants
@carusogabriel @Majkl578 @kukulich @Ocramius @deeky666 @alcaeus @greg0ire
0