Skip to content
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

Removed closing PHP tag due to PSR-2 definition #14929

Merged
merged 1 commit into from Oct 3, 2019
Merged

Removed closing PHP tag due to PSR-2 definition #14929

merged 1 commit into from Oct 3, 2019

Conversation

Crease29
Copy link

@Crease29 Crease29 commented Oct 1, 2019

While reading your Matomos contributing guide, I saw, that Matomo is sticking to PSR-1, PSR-2 and PSR-4. I digged into the code and saw some closing PHP tags, which must be omitted (see: https://www.php-fig.org/psr/psr-2/#22-files).

There are also closing PHP tags in libs/HTML but I wasn't sure, if these should be touched.

@tsteur
Copy link
Member

tsteur commented Oct 1, 2019

@Crease29 thanks for this. You updated the custom alerts submodule which makes the tests fail see https://travis-ci.org/matomo-org/matomo/jobs/592136762#L533 could you revert the submodule change?

@Crease29
Copy link
Author

Crease29 commented Oct 3, 2019

Sorry for the amount of pushes. I messed something up here when reverting the previous commit. Should be fine now 🙂

Edit: I'd also like to fix some other PHPDoc and code style issues. Is that something that is welcome or wouldn't that be of value for you?

@tsteur
Copy link
Member

tsteur commented Oct 3, 2019

Cheers @Crease29 for fixing the PR will merge now.

Sure we appreciate such PRs as long as they don't change any logic and are easy to review and quick to merge before any merge conflicts happen :)

@tsteur tsteur merged commit b64835b into matomo-org:3.x-dev Oct 3, 2019
@Crease29 Crease29 deleted the psr-2-remove-closing-php-tag branch October 4, 2019 06:40
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.

None yet

2 participants