Skip to content

Conversation

@desrosj
Copy link
Member

@desrosj desrosj commented Dec 18, 2018

When grunt build is run in WordPress core, the parser is copied to the appropriate place in the core code base. But, because these lines are not aligned correctly, the wp-includes/class-wp-block-parser.php file always shows changes.

This fix will prevent that from happening.

See the Slack discussion for more information.

@desrosj desrosj self-assigned this Dec 18, 2018
$this->blockName = $name;
$this->attrs = $attrs;
$this->innerBlocks = $innerBlocks;
$this->innerHTML = $innerHTML;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why this wasn't caught by the phpcs config in Gutenberg? should we update it?

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 Good question. Let me dig in a bit. I am not familiar with the plugin's ruleset.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, now that I look, there are a number of PHPCS issues in this file. Wonder if it's running against it at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is <exclude-pattern>./lib/parser.php</exclude-pattern> in the PHPCS file, but I don't think this is excluding this specific file.

I think that the file is just not being processed from lack of inclusion rather than exclusion. Replacing all the <file></file> tags and replacing them with just <file>.</file> (all files) correctly formats this file, but also a handful of others (but all other files are e2e or phpunit test related).

If possible, I'd like to merge this change, and explore modifying the ruleset in a separate PR.

@youknowriad youknowriad added this to the 4.8 milestone Dec 18, 2018
Copy link
Contributor

@earnjam earnjam left a comment

Choose a reason for hiding this comment

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

Just an alignment fix 👍

@youknowriad
Copy link
Contributor

Also would be good if we create a corresponding issue in the 5.0.3 milestone. That way we don't forget to include it in the next package release.

@earnjam
Copy link
Contributor

earnjam commented Dec 18, 2018

So it looks like we aren't running PHPCS against any packages except block-library

from phpcs.xml.dist:

	<file>./bin</file>
	<file>./packages/block-library/src</file>
	<file>./lib</file>
	<exclude-pattern>./lib/parser.php</exclude-pattern>
	<file>./phpunit</file>
	<file>gutenberg.php</file>

Changing that to run against all packages kicks errors in the block-serialization-default-parser and block-serialization-spec-parser packages. How should we handle that @youknowriad? The former looks like it has a bunch of issues to be corrected.

@youknowriad
Copy link
Contributor

Can we add the package the phpunit config and fix the issues? Is it too much work?

@gziolo gziolo added the [Type] Code Quality Issues or PRs that relate to code quality label Dec 19, 2018
@gziolo gziolo merged commit 5f683de into WordPress:master Dec 19, 2018
@gziolo
Copy link
Member

gziolo commented Dec 19, 2018

Can we add the package the phpunit config and fix the issues? Is it too much work?

It's already tracked in #13002 which is assigned to 5.0.3 Milestone :)

@desrosj desrosj deleted the fix/incorrect-spacing-block-serialization-parser branch December 19, 2018 17:05
@desrosj
Copy link
Member Author

desrosj commented Dec 19, 2018

I believe that this did not make it in time for the package releases for 5.0.2, so moving this to the next milestone. If that is not true, feel free to move it back.

@desrosj desrosj modified the milestones: 4.8, 4.9 Dec 19, 2018
@youknowriad youknowriad modified the milestones: 4.9, 4.8 Dec 19, 2018
@youknowriad
Copy link
Contributor

This will be 4.8 actually which is the current (closed scope) milestone.

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

Labels

[Type] Code Quality Issues or PRs that relate to code quality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants