Users with numeric usernames about ccdnuserprofilebundle HOT 9 CLOSED

Maybe split the controller action into 2?

And make the routes like:
/en/profile/id/123
/en/profile/name/tom

Thing to be aware of though is that usernames can change, if the user bundle allows it of course its just a case of editing ones account and changing username, which is why i prefer to address the profile by id.

from ccdnuserprofilebundle.

Good point, I'd definitely favour profile lookup by id then.

The only problem is: this behaviour has been exposed. Any applications using the bundle might have exposed URLs such as /en/profile/tom. If so, this will create a backwards-compatibility break, as the URLs will no longer work.

from ccdnuserprofilebundle.

URLs should be generated via routes, devs should not be hard-coding urls. Only thing affected really is google caching etc.

As the routes are already named as *_by_id then its just a case of changing the route from:

ccdn_user_profile_show_by_id:
    pattern:  /{userId}
    defaults: { _controller: CCDNUserProfileBundle:Profile:showOverview, userId: 0, _locale: en }

to

ccdn_user_profile_show_by_id:
    pattern:  /id/{userId}
    defaults: { _controller: CCDNUserProfileBundle:Profile:showOverview, userId: 0, _locale: en }

ccdn_user_profile_show_by_name:
    pattern:  /name/{userName}
    defaults: { _controller: CCDNUserProfileBundle:Profile:showOverviewByName, _locale: en }

Though, tbh, we can forgo the option to list by username altogether if its easier, though its a nice way to find a profile manually in the quickest way though it does require an additional method, or we can use the same method, with 2 variables $id, and $name and prioritise name over id when set to determine model method for profile retrieval.

from ccdnuserprofilebundle.

Assuming experienced Symfony 2 developers, then yes, URLs will never be hardcoded. But with the Router, you can generate a faulty URL:

// Inside a container-aware class.
// Outputs: /app_dev.php/en/profile/tom
echo $this->container
    ->get('router')
    ->generate('ccdn_user_profile_show_by_id', ['userId' => 'tom']);

So the possibility exists that someone uses the URL in their own application. If the possibility exists, isn't it a BC break?

In any case, maybe we should keep it simple for the moment:

• Find by Id, but not by name. It's useful, but too likely to change. And search functionality is much more common for that use case.
• Same with keeping the URL as-is, but prioritising one over the other. As long as usernames can be the same as a user's identifier, then there is room for confusion.

from ccdnuserprofilebundle.

I doubt it honestly. The variable states ID, so i doubt anyone is using username in there as i don't think its mentioned in the docs that you can.

I think its better to have a BC break at this early point to get it right rather than wait for more people to use it and compound the issue.

from ccdnuserprofilebundle.

That's a good assertion, I think. All right, so let's:

• replace all calls of the generic find method to use the find-by-id method, and,
• remove the generic find method.

from ccdnuserprofilebundle.

Sounds good. 👍

from ccdnuserprofilebundle.

I'll close this ticket, as it's fixed in the merged PR (next time I'll need to find a way of attaching PRs to existing tickets)

from ccdnuserprofilebundle.

@maartenJacobs

next time I'll need to find a way of attaching PRs to existing tickets

yeah you can reference as many ticket-numbers as you like from a PR either in the commit message or here in the discussion by prefixing them with a hash symbol, github will see them and link it all up for you.

from ccdnuserprofilebundle.