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

Overriding entity doesn't work #55

Open
lukepass opened this issue Mar 5, 2019 · 18 comments
Open

Overriding entity doesn't work #55

lukepass opened this issue Mar 5, 2019 · 18 comments

Comments

@lukepass
Copy link

lukepass commented Mar 5, 2019

Hello,
I tried overriding the Notification entity following your guide:

<?php

namespace AppBundle\Entity;

use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Validator\Constraints as Assert;
use Mgilet\NotificationBundle\Entity\NotificationInterface;
use Mgilet\NotificationBundle\Model\Notification as NotificationModel;

/**
 * Notification
 *
 * @ORM\Entity(repositoryClass="AppBundle\Repository\NotificationRepository")
 */
class Notification extends NotificationModel implements NotificationInterface
{
    /**
     * @var string
     *
     * @ORM\Column(name="`type`", type="string", length=255)
     */
    private $type;

    /**
     * @var string|null
     *
     * @ORM\Column(name="sender", type="string", length=255, nullable=true)
     */
    private $sender;

    /**
     * Set type.
     *
     * @param string $type
     *
     * @return Notification
     */
    public function setType($type)
    {
        $this->type = $type;

        return $this;
    }

    /**
     * Get type.
     *
     * @return string
     */
    public function getType()
    {
        return $this->type;
    }

    /**
     * Set sender.
     *
     * @param string|null $sender
     *
     * @return Notification
     */
    public function setSender($sender = null)
    {
        $this->sender = $sender;

        return $this;
    }

    /**
     * Get sender.
     *
     * @return string|null
     */
    public function getSender()
    {
        return $this->sender;
    }
}

Unfortunately it's not working as it tries to create the table twice:

mysite git:(master) ✗ sf doctrine:schema:update --dump-sql | highlight -l sql

In SchemaException.php line 108:
                                                                 
  The table with name 'mysite.notification' already exists.  
                                                                 

doctrine:schema:update [--complete] [--dump-sql] [-f|--force] [--em [EM]] [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-e|--env ENV] [--no-debug] [--] <command>

Looking at the output it's trying to execute the query twice. This is my config:

    orm:
        auto_generate_proxy_classes: '%kernel.debug%'
        naming_strategy: doctrine.orm.naming_strategy.underscore
        auto_mapping: true
        dql:
            numeric_functions:
                rand: DoctrineExtensions\Query\Mysql\Rand
                orm:
        resolve_target_entities:
            Mgilet\NotificationBundle\Entity\Notification: AppBundle\Entity\Notification

Thanks!

@maximilienGilet
Copy link
Owner

Hi @lukepass ! Thanks for using the bundle !

At first glance it seems like your configuration has a typo : inside the numeric_functions node after your rand function, you have an orm which looks like a bad copy-paste.

It should work if you remove the empty orm: line

@lukepass
Copy link
Author

lukepass commented Mar 5, 2019

Hello @maximilienGilet and thanks for the fast response!
Unfortunately that was just a typo but the error is still the same:

The table with name 'mysite.notification' already exists.

Now I am using an entity called AppNotification and it seems to work but I had to change the table name:

<?php

namespace AppBundle\Entity;

use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Validator\Constraints as Assert;
use Mgilet\NotificationBundle\Entity\NotificationInterface;
use Mgilet\NotificationBundle\Model\Notification as NotificationModel;

/**
 * AppNotification
 *
 * @ORM\Entity(repositoryClass="AppBundle\Repository\NotificationRepository")
 * @ORM\Table(name="app_notification")
 */
class AppNotification extends NotificationModel implements NotificationInterface
{
    const TYPE_SYSTEM = 'system';
    const TYPE_ACHIEVEMENT = 'achievement';
    const TYPE_CUSTOM = 'custom';

    /**
     * @var string
     *
     * @ORM\Column(name="`type`", type="string", length=255)
     */
    private $type;

    /**
     * @var string|null
     *
     * @ORM\Column(name="sender", type="string", length=255, nullable=true)
     */
    private $sender;

    /**
     * Set type.
     *
     * @param string $type
     *
     * @return AppNotification
     */
    public function setType($type)
    {
        $this->type = $type;

        return $this;
    }

    /**
     * Get type.
     *
     * @return string
     */
    public function getType()
    {
        return $this->type;
    }

    /**
     * Set sender.
     *
     * @param string|null $sender
     *
     * @return AppNotification
     */
    public function setSender($sender = null)
    {
        $this->sender = $sender;

        return $this;
    }

    /**
     * Get sender.
     *
     * @return string|null
     */
    public function getSender()
    {
        return $this->sender;
    }
}
    orm:
        auto_generate_proxy_classes: '%kernel.debug%'
        naming_strategy: doctrine.orm.naming_strategy.underscore
        auto_mapping: true
        dql:
            numeric_functions:
                rand: DoctrineExtensions\Query\Mysql\Rand
        resolve_target_entities:
            Mgilet\NotificationBundle\Entity\Notification: AppBundle\Entity\AppNotification

Is this how it's supposed to work?
It's still creating the notification table (now I have notification and app_notification tables) but leaving notification empty.

Thanks!

@emulienfou
Copy link
Contributor

Hi @lukepass didn't test it yet, but maybe the Single table inheritance could fix this issue.

However, it seems this annotations need to be added in the Notification entity class inside the NotificationBundle. If this is the case and it works, you should submit a pull-request.

@emulienfou
Copy link
Contributor

Hi @maximilienGilet, so clearly there is an issue right here with the bundle when you are trying to override the default entities.
With the actual code, Doctrine find 2 entities with the same table name notification and throw an exception.

The best way should be to make do a refactoring and provide 3 models classes (Notification, NotifiableEntity and NotifiableNotification).

For Symfony Flex users, it will be perfect to create a recipe who provide the entity classes.
However for the others they will need to create the 3 entity classes manually.

@maximilienGilet What do you think about this idea?

@maximilienGilet
Copy link
Owner

Hi @emulienfou this is a great idea !

However I don't use the bundle anymore and don't have much time to make the changes...

Make me a pull request and I'll try and merge it :)

@lukepass
Copy link
Author

lukepass commented Apr 1, 2019

Unfortunately I don't use Flex or Symfony 4. I tried with the single table inheritance but it didn't work well.
For my application I solved the problem just by making another entity with a different table name (see my previous answer) and it's working quite fine. The only drawback is that there is an additional empty notification table.
I agree with @emulienfou solution.

@emulienfou
Copy link
Contributor

Hi @lukepass, I'm currently using SF4 so it would be great if you can test it on your Symfony version.

However, you will need to use my repository and add the following lines to your composer.json:

{
    "repositories": [
        { "type": "vcs", "url": "https://github.com/emulienfou/notification-bundle" }
    ]
}

Updated documentation to create entities and add Doctrine resolving mapping is here: https://github.com/emulienfou/notification-bundle/blob/master/Resources/doc/index.rst#add-entity-classes-non-symfony-flex-user

Let me know if there is an issue on your side, I will make some fixes before creating the Pull Request and adding the Symfony Flex recipe

@danielrestrepo
Copy link

Hi @emulienfou is your version overriding the entities right with SF4?

@emulienfou
Copy link
Contributor

Hi @danielrestrepo exactly, the updated code do not provide entities anymore, only mapped superclasses.
You will need to add 3 entities in your main project who extends the model classes, like provided by the Symfony Flex Recipe.

Do not forget to add the doctrine configuration too, with the resolve_target_entities entries.

@danielrestrepo
Copy link

danielrestrepo commented Apr 2, 2019

Thanks a lot @emulienfou custom mapping works now but I wanted to use the NotificationManager and I ran into a couple of issues as accessor methods are expecting the abstract class and not the interface like here: https://github.com/emulienfou/notification-bundle/blob/master/Model/NotificationInterface.php#L78

If I use there the NotifiableNotificationInterface error would go away. Am I right? Can I send you a PR with those changes?

@emulienfou
Copy link
Contributor

Hi @danielrestrepo just made the changes to use the interface instead of the model class.
Let me know if there is more issues.
I will take some times to check if there is other issues too

@danielrestrepo
Copy link

danielrestrepo commented Apr 2, 2019

Thanks @emulienfou I'll update from your last commit and test again. For now the only additional thing I found is that Notification entity didn't persist but NotifiableNotificationand Notifiable did when using this code:

                $notification = $this->notification->createNotification('Proceso asignado');
                $notification->setMessage('EL proceso xxx del 2019-04-02 le ha sido asignado');
                $notification->setDate(new \DateTime());

                $stepEvents = $event->getFlowEvents();

                $user = null;
                foreach ($stepEvents as $stepEvent) {
                    if ($stepEvent instanceof AssignUserEvent) {
                        /** @var UserInterface $user */
                        $user = $this->repositoryRegistry->getUserRepository()->find($stepEvent->getUserId()->toString());
                    }
                }

                $this->notification->addNotification([$user], $notification, true);

Using original bundle without custom mappings was working with the same code. Not sure if it is the NotificationManager. Any ideas?

Update

I just needed to add cascade={"persist"} on the owning side of the relationship so please ignore this question.

Thanks anyway. It's been a pleasure collaborating with you

@emulienfou
Copy link
Contributor

Hi @danielrestrepo I apply the changes your made using interfaces in all models.
Glad you figured out and made this bundle working on your side

@danielrestrepo
Copy link

Hi @emulienfou got back to your fork and is working nicely. Thanks a lot. I'll let you know if found something worth mentioning.

@aquibbaig
Copy link

aquibbaig commented Jun 5, 2019

Hello, I tried overriding the existing Notifications entity with a new class in my src/Entity as mentioned in the docs. And the table also got updated to show my latest changes.
However when I try to call upon setters and getters for the new Entity in which I added a few fields I get the following error:

Attempted to call an undefined method named "setType" of class "Mgilet\NotificationBundle\Entity\Notification".

1. Here is the overridden entity

<?php

namespace App\Entity;

use Doctrine\ORM\Mapping as ORM;
use Mgilet\NotificationBundle\Entity\NotificationInterface;
use Mgilet\NotificationBundle\Model\Notification as NotificationModel;

/**
 * @ORM\Entity(repositoryClass="Mgilet\NotificationBundle\Entity\Repository\NotificationRepository")
 *
 */
class Notification extends NotificationModel implements NotificationInterface
{
    /**
     * Overrides the existing Notifications entity
     *
     * @ORM\Column(name="type", type="string", length=255, nullable=false)
     */
    protected $type;

    /**
     * Get the Notification type
     *
     * @return string
     */
    public function getType() : string {
        return $this->type;
    }

    /**
     * Set the Notification type
     *
     * @param $type
     */
    public function setType($type){
        $this->type = $type;
    }
}

2. Here is doctrine.yml

orm:
        auto_generate_proxy_classes: "%kernel.debug%"
        auto_mapping: true
        mappings:
            App:
                is_bundle: false
                type: annotation
                dir: '%kernel.project_dir%/src//Entity'
                prefix: 'App\Entity'
                alias: App
        resolve_target_entities:
            Mgilet\NotificationBundle\Entity\Notification: App\Entity\Notification

3. Here is what I try to achieve in my Controller

$manager = $this->get('mgilet.notification');
            $notif = $manager->createNotification('Project Creation');
            $notif->setMessage('Your project '.$p->getDisplayName().' was created!');
            $notif->setLink('Link here');
            $notif->setType('web');

@maximilienGilet @emulienfou
But it doesn't work. Some help here would be very useful as I am running late for my major projects

@aquibbaig
Copy link

So, it worked because you have to create an Instance of the new overridden entity rather than creating the other entity. That fixed it for me

@marcin-dorosiewicz
Copy link

Hi guys,
@aquibbaig how did you solve this problem? I have exactly the same.
How to extend NotificationManager and don't lose events and all useful things but work on extended entity?

@aquibbaig
Copy link

Hi @marcin-dorosiewicz, you can follow up to Step No 2 here. After that in the controller use dependency injection to get the overridden notification class. Create an instance of the overridden notification class. And pass it into $manager->addNotification().

However, if it is of no use, you can post the exact problem here to be of more help. Good luck!

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

No branches or pull requests

6 participants