-
-
Notifications
You must be signed in to change notification settings - Fork 79
Comment feature #2
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
Conversation
View/post.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use the & HTML entity & instead?
Model/Comment.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be better to extend this class to Blog (similar to the Admin model) you can can get rid of the constructor).
Model/Comment.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, extend it to Blog class Comment extends Blog
Model/Comment.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then, if so, remove $oDb property
Controller/Comment.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here. Might be a good idea to extend to Blog, so you won't have to initialize session and you will be able to reuse the parent $oUtil object.
And call the parent constructor in Comment constructor.
Controller/Comment.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then, maybe rename $oModel to oCommentModel to avoid confusion with its parent Blog class
Hi Serge! I really appreciate your PR! 😄 Really nice changes overall!
Nice to see you keep the same coding conventions for consistency!
Please find a few notes/suggestions
That looks good to me @Kishiko
I don't have time to test your comment feature right now. Just to make sure before merging your branch.
Did you do some smoke testing? Does it work fine?
Thanx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure you will need $this->oUtil->getModel('Comment'); after $this->oUtil->getModel('Blog'); in order to include the file, since the SPL autoload isn't done for the Model classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you done what I tell you in my previous above comment, you can just remove the constructor here, so PHP will automatically call the parent constructor
Let me know if everything works well after my suggestion @Kishiko?
@Kishiko Hope everything is fine? Let me know if my change request was clear for you?
Hey @Kishiko Let me know if you get a chance to read my request.
Otherwise, I will have to close your great changes, unfortunately.
I've added the comment feature. So, everyone can be able to comment a post by writing down its name and the content of its comment.