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

[Product Component] no setter for id #1408

Closed
harikt opened this issue Apr 19, 2014 · 6 comments
Closed

[Product Component] no setter for id #1408

harikt opened this issue Apr 19, 2014 · 6 comments
Labels
Help Wanted Issues needing help and clarification.

Comments

@harikt
Copy link
Contributor

harikt commented Apr 19, 2014

Hi guys,

Was working with the product component and noticed there is no setter for id . How are you working on setting the id back when a Product is loaded or is saved.

$product = new Sylius\Component\Product\Model\Product();
$product->setName('My awesome shirt');
$product->setSlug('my-awesome-shirt');
$product->setDescription('My awesome shirt description');
$product->setMetaKeywords('awesome, shirt');
$product->setMetaDescription('meta description');
$product_service->save($product);

From the product_service I would like to save the id back to the Product object. Now the id is protected and there is no setter for it . In doctrine yes it works! , but for other db related we need a setter or it should be public.

Thanks.

@winzou
Copy link
Contributor

winzou commented Apr 21, 2014

Hi @harikt

If you need this getId method, you have to use your own model class, in which you can define this method. This is really easy to do in sylius.

@harikt
Copy link
Contributor Author

harikt commented Apr 21, 2014

@winzou if you are saying extend the class and add the setId method, ok.

But if you don't have a setter for id the goal of making components don't fit completely, for many of them may not be going to use Doctrine or Symfony. They may be going to use PDO or some other wrappers, like me doing with Aura.Sql .

Say $product_repository->find(1); here you need to set the values of the name, description, id etc ;) . This is where I was mentioning about setId . I know doctrine don't like setId method this is why are omitting.

@docteurklein
Copy link
Contributor

I think there is something else that should be taken into account: encapsulation.

It wouldn't be good oop to provide an interface that allows to arbitrary change the id of an object from the outside.

The id setting should be delegated either as a private detail of impl, or passed in the ctor.

@harikt
Copy link
Contributor Author

harikt commented Jul 28, 2014

@docteurklein may be I don't know, so would love to learn. Let us take a simple example of Post class.

<?php
class Post
{
    private $id;
    private $title;
    private $body;
    public function __construct($id = null, $title = null, $body = null)
    {
        $this->id = $id;
        $this->title = $title;
        $this->body = $body;
    }
    public function getId()
    {
        return $this->id;
    }
    public function getTitle()
    {
        return $this->title;
    }
    public function getBody()
    {
        return $this->body;
    }
    public function setTitle($title)
    {
        $this->title = $title;
    }
    public function setBody($body)
    {
        $this->body = $body;
    }
}

Now consider you are fetching via Repository .

$post_repository->find(1);

You can set the id to the Post object on constructor time, for in the mapper you may be getting the value of id.

Now consider you are saving.

When you save either you want to use Reflection to set the id value which you got from the mapper.

Or do you find some other way without a setter like

    public function setId($id)
    {
        $this->id = $id;
    }

@stloyd
Copy link
Contributor

stloyd commented Oct 1, 2014

I think this can be closed.

id value is totally internal data that should be never changed (as it's internal value) that "binds" objects together in case of relations etc.

If you really need some product identifier you should look at sku (I can agree it should be maybe moved from Core into Product component - @pjedrzejewski ?), and adjust your repository to look at this value while searching instead of id:

$variant = new Sylius\Component\Core\Model\ProductVariant();
$variant ->setSku('YOUR_RANDOM_SKU_DATA');

$product = new Sylius\Component\Product\Model\Product();
$product->setName('My awesome shirt');
$product->setMasterVariant($variant);

$service->save($product);

$variant = $repository->findOneBy(array('sku' => 'YOUR_RANDOM_SKU_DATA'));
$variant->getProduct();

@harikt
Copy link
Contributor Author

harikt commented Oct 1, 2014

@stloyd ok that moves to not using the same mysql id and making use of something like uuid.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help Wanted Issues needing help and clarification.
Projects
None yet
Development

No branches or pull requests

5 participants