quickapps / cms

Modular CMS powered by CakePHP
GNU General Public License v3.0
164 stars 69 forks source link

Fixed profile page and routing #159

Closed ashikkalavadiya closed 7 years ago

ashikkalavadiya commented 8 years ago

I have removed get() function as it fires an error with NULL primary key. So i replace with find(). Also route isn't having parameter with $id.

botchris commented 7 years ago

Hi,

We use get() because it's expected to throw, from its docblock: "RecordNotFoundException When user not found, or users has marked profile as private"

Throwing RecordNotFoundException will cause to send a 404 HTTP code, which is expected under production (if debug enable you will see an "Internal Error" message which is just ok).

We should still throwing this exception, however it's possible to define a more appropiate message for it as yours ("Profile not found" or whatever). Remeber people can reach this profile URL from an external site, causing a redirection to nowhere, and thus flash message never to be delivered.


In the other hand, as you said, there a problem with the routes, but they should look as follow:

Router::connect('/user/profile/:id', [
    'plugin' => 'User',
    'controller' => 'Gateway',
    'action' => 'profile',
], [
    'routeClass' => 'Cake\Routing\Route\InflectedRoute',
    'id' => '\d+',
    'pass' => ['id'],
]);

Router::connect('/admin/user/profile/:id', [
    'plugin' => 'User',
    'controller' => 'Gateway',
    'action' => 'profile',
    'prefix' => 'admin',
], [
    'routeClass' => 'Cake\Routing\Route\InflectedRoute',
    'id' => '\d+',
    'pass' => ['id'],
]);

You can apply this patch on this same PR if you want to.


Thanks for reporting :smile: Best regards!

ashikkalavadiya commented 7 years ago

Yeah, i will fix it! Just to inform, if current user visit /admin/user/profile . He/she would be able to see own's profile.

As your suggested route entry we have to pass :id param.

Regarding redirection if profile is not public or his/her own profile. Where we have to redirect??? I have found a reference from /user/me, so i have redirected to refer URL.

Thanks for duggestion and maintaining a repo!

Happy to help :) Ashik

botchris commented 7 years ago

You mean with no argument? In that case you would need to make the $id parameter optional. But if you do, you will get a "NULL primary key" for anonymous users as their ID is actually NULL. So you would need to tweak the profile() action a bit.

I think the following changes should work (NO TESTED):

/**
 * Shows profile information for the given user.
 *
 * @param null|int $id User's ID, or null to use current user's ID. // 0: Update docblock
 * @return void
 * @throws \Cake\ORM\Exception\RecordNotFoundException When user not found, or
 *  users has marked profile as private
 */
public function profile($id = null)    // 1: Make $id optional
{
    $this->loadModel('User.Users');
    $conditions = [];
    $id = $id === null ? user()->id : $id; // 2: Get current user's id if null

    if ($id != user()->id) {
        $conditions = ['status' => 1, 'public_profile' => true];
    }

    $user = $this->Users->get((int)$id, ['conditions' => $conditions]); // 3: Cast to integer to ensure a valid value

    $this->title(__d('user', 'User’s Profile'));
    $this->viewMode('full');
    $this->set(compact('user'));
}

In the other hand be aware that /user/profile & /user/profile/ are not the same, if you want to allow both then you should preserve existing routes and add the others two using the :id.

Something like this (NO TESTED neither):

// preserve this two routes:
Router::connect('/user/profile', [
    'plugin' => 'User',
    'controller' => 'Gateway',
    'action' => 'profile',
], ['routeClass' => 'Cake\Routing\Route\InflectedRoute']);

Router::connect('/admin/user/profile', [
    'plugin' => 'User',
    'controller' => 'Gateway',
    'action' => 'profile',
    'prefix' => 'admin',
], ['routeClass' => 'Cake\Routing\Route\InflectedRoute']);

// add this two new routes:
Router::connect('/user/profile/:id', [
    'plugin' => 'User',
    'controller' => 'Gateway',
    'action' => 'profile',
], [
    'routeClass' => 'Cake\Routing\Route\InflectedRoute',
    'id' => '\d+',
    'pass' => ['id'],
]);

Router::connect('/admin/user/profile/:id', [
    'plugin' => 'User',
    'controller' => 'Gateway',
    'action' => 'profile',
    'prefix' => 'admin',
], [
    'routeClass' => 'Cake\Routing\Route\InflectedRoute',
    'id' => '\d+',
    'pass' => ['id'],
]);
ashikkalavadiya commented 7 years ago

It seems that router wont work as you can not define same action in multiple router!

You can merge PR, only redirection to refer URL seems something bit wrong! Have you tested with my code??

If you will pass id then it will test with condition otw it checks for public profile availability!

I have corrected other things in this repo. Will do pull requests later on..

botchris commented 7 years ago

closing, solved at ab022afaadb62191697a5e6d7abbc05e1306906c