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

Comment feature #2

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

Comment feature #2

wants to merge 5 commits into from

Conversation

Kishiko
Copy link

@Kishiko Kishiko commented Mar 7, 2018

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.

View/post.php Outdated
<?php endif ?>

<h4>Add a new comment</h4>
<form action="<?=ROOT_URL?>?p=comment&a=add" method="post">
Copy link
Owner

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 &amp; instead?


public function __construct()
{
$this->oDb = new \TestProject\Engine\Db;
Copy link
Owner

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).


namespace TestProject\Model;

class Comment
Copy link
Owner

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


class Comment
{
protected $oDb;
Copy link
Owner

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


namespace TestProject\Controller;

class Comment
Copy link
Owner

@pH-7 pH-7 Mar 8, 2018

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.


/** Get the Model class in all the controller class **/
$this->oUtil->getModel('Comment');
$this->oModel = new \TestProject\Model\Comment;
Copy link
Owner

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

@pH-7
Copy link
Owner

pH-7 commented Mar 8, 2018

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

@pH-7
Copy link
Owner

pH-7 commented Mar 22, 2018

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

@@ -26,6 +26,7 @@ public function __construct()
/** Get the Model class in all the controller class **/
$this->oUtil->getModel('Blog');
Copy link
Owner

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.

{
public function __construct()
{
$this->oUtil = new \TestProject\Engine\Util;
Copy link
Owner

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

@pH-7
Copy link
Owner

pH-7 commented Mar 24, 2018

Let me know if everything works well after my suggestion @Kishiko?

@pH-7 pH-7 assigned Kishiko and unassigned Kishiko Mar 27, 2018
@pH-7
Copy link
Owner

pH-7 commented Mar 30, 2018

@Kishiko Hope everything is fine? Let me know if my change request was clear for you?

@pH-7
Copy link
Owner

pH-7 commented Apr 10, 2018

Hey @Kishiko Let me know if you get a chance to read my request.
Otherwise, I will have to close your great changes, unfortunately.

@pH-7 pH-7 assigned Kishiko and unassigned Kishiko Jan 17, 2019
@pH-7 pH-7 assigned Kishiko and unassigned Kishiko Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants