Replace substr in getCode() to cover more cases. #61
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current replacer does not cover all cases to remove declare statements regarding 'strict_types' definitions. Currently not covered: * If there is more than one line break before the declare statement * If there are arbitrary spaces within the declare statement * If there is a comment before the declare statement To not build the whole statemachine by ourself I decided to use a regex to tackle those problems. As this will in most cases only be ran if the boostrap needs to be regenerated we should be safe with the implied performance hit regarding the usage of regex here. Rundown on the Regex parts: * `^` - Match the beginning of the file only * `(<\?php)?` - Match starting PHP Tags (if existent) * `([\s]*/\*\*?.*?\*/[\s]*)?` - Match file comments (if existent) * `[\s]*` - Match arbitrary number of spaces * `(declare[\s]*\([\s]*strict_types[\s]*=[\s]*1[\s]*\);)?` - Match strict definition (if existent) This should handle the whole pretty cleanup more gracefully regarding external/vendored codebases which may not apply to some predefined CodingStandard.
Contributor
Author
|
Not throughly tested as its broken atm. and it removes too much |
Because of the multiline modifier we matched everything within the whole file and not just from beginning. This resulted in removing all comments and whitespaces -- whoops!
Member
|
Thanks! Please could you also add a test case that oreviously failed, but now passes? |
Contributor
Author
|
Tests were added and I also found a missing spacing part in the regex. |
Those should not be checked for styling though!
This should fix PHP Parser <3.0 errors.
Contributor
Author
|
Hey there, I don't want to rush you, but could you do a review and merge this upstream so I can use this library directly instead of patching my composer.json with my patch repository/branch? |
Member
|
Thanks. 👍 |
Member
|
Just tagged 3.2.0. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current replacer does not cover all cases to remove declare statements regarding 'strict_types' definitions.
Currently not covered:
To not build the whole statemachine by ourself I decided to use a regex to tackle those problems. As this will in most cases only be ran if the boostrap needs to be regenerated we should be safe with the implied performance hit regarding the usage of regex here.
Rundown on the Regex parts:
^- Match the beginning of the file only(<\?php)?- Match starting PHP Tags (if existent)([\s]*/\*\*?.*?\*/[\s]*)?- Match file comments (if existent)[\s]*- Match arbitrary number of spaces(declare[\s]*\([\s]*strict_types[\s]*=[\s]*1[\s]*\);)?- Match strict definition (if existent)This should handle the whole pretty cleanup more gracefully regarding external/vendored codebases which may not apply to some predefined CodingStandard.