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

comments added for learning purposes. #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tistudios
Copy link

added closing php tag

added closing php tag
@wolveix
Copy link

wolveix commented Jan 22, 2019

Your request changes one of the quotation marks to an apostrophe, but not the ending mark. These should both be changed since interpolation is not occurring within that string. Additionally, a closing PHP tag isn't strictly necessary and there are various positive points supporting leaving the tag out within a PHP-only file.

@tistudios
Copy link
Author

tistudios commented Jan 28, 2019

The first one was made in error. Thanks for catching that.

Re 2nd point, thanks for letting me know its considered poor practice.
Maybe others could make the same mistake if they base it on assumption, I will add a comment with a link to some documentation. Thanks Wolveix.

Copy link
Author

@tistudios tistudios left a comment

Choose a reason for hiding this comment

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

Amended.

@tistudios tistudios changed the title Update TimeActivityRead.php comments added for learning purposes. Jan 28, 2019
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.

2 participants