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

Support Custom Foreign Keys #53

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

inmanturbo
Copy link

@inmanturbo inmanturbo commented Mar 26, 2024

Hey there! This PR adds support for custom foreign keys.

For instance if someone needs to use a uuid but it's not the key on the model.

This will make it easier to track userstamps across distributed systems.

In our case users are in a seperate database from most resources. Users have a ulid column specifically for relationships outside of the Auth domain, and for showing links where we won't show the auto-incremented id, which is used internally within the Auth domain.

This isn't a breaking change because it uses the default by default

    /**
     * Get the name of the foreign "id" column for the creator.
     *
     * @return string
     */
    public function getCreatedByKey()
    {
        return defined('static::CREATED_BY_KEY') ? static::CREATED_BY_KEY : (new ($this->getUserClass())->getKeyName();
    }

Usage

use Wildside\Userstamps\Userstamps;

class Foo extends Model {

    use Userstamps;

    const CREATED_BY = 'created_by_uuid';
    const CREATED_BY_OWNER_KEY = 'uuid';
    const UPDATED_BY = 'updated_by_uuid';
    const UPDATED_BY_OWNER_KEY = 'uuid';
    const DELETED_BY = 'deleted_by_uuid';
    const DELETED_BY_OWNER_KEY = 'uuid';
}
  • Edited to reflect current changes

@mattmcdonald-uk
Copy link
Member

Thanks for the PR.

For this to truly not be a breaking change, the current value passed along to the ownerKey property is null, so the default returned value should be null.

I would also prefer naming the const CREATED_BY_OWNER_KEY, etc. to sync with the argument that's being sent to belongsTo, and the methods similarly being getCreatedByOwnerKey, etc.

@inmanturbo
Copy link
Author

Thanks for the PR.

For this to truly not be a breaking change, the current value passed along to the ownerKey property is null, so the default returned value should be null.

I would also prefer naming the const CREATED_BY_OWNER_KEY, etc. to sync with the argument that's being sent to belongsTo, and the methods similarly being getCreatedByOwnerKey, etc.

Thank you. After I've finished making the changes mentioned above I'll mark this as ready for review. Leaving as draft until then.

@inmanturbo inmanturbo marked this pull request as ready for review March 26, 2024 17:21
@inmanturbo
Copy link
Author

@mattmcdonald-uk I've made the requested changes and marked this ready. Please review when you get a chance, thanks.

@inmanturbo
Copy link
Author

If you would prefer, I'm not opposed to opening a new Pr as a single commit. I've resisted squashing too much here, so that the conversation would be easier to follow.

@inmanturbo inmanturbo marked this pull request as draft March 26, 2024 19:03
@inmanturbo
Copy link
Author

inmanturbo commented Mar 26, 2024

I've overlooked the fact the the Listeners are all using Auth::id(), marking as draft for now until this is fixed.

@inmanturbo inmanturbo marked this pull request as ready for review March 26, 2024 19:22
@inmanturbo
Copy link
Author

@mattmcdonald-uk it's ready!

@inmanturbo
Copy link
Author

inmanturbo commented Mar 29, 2024

If anyone is interested in trying out this feature, you can do so by temporarily adding the fork to the repositories property of your composer.json

{
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/inmanturbo/Laravel-Userstamps.git"
        }
    ]
}

Then update your dependency constraint to reference this branch by running:

composer require wildside/userstamps:dev-patch-1

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