notification-bundle icon indicating copy to clipboard operation
notification-bundle copied to clipboard

Overriding entity doesn't work

Open lukepass opened this issue 5 years ago • 19 comments

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!

lukepass avatar Mar 05 '19 13:03 lukepass

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

maximilienGilet avatar Mar 05 '19 13:03 maximilienGilet

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!

lukepass avatar Mar 05 '19 13:03 lukepass

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 avatar Mar 10 '19 22:03 emulienfou

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?

emulienfou avatar Mar 22 '19 21:03 emulienfou

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 :)

maximilienGilet avatar Mar 25 '19 07:03 maximilienGilet

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.

lukepass avatar Apr 01 '19 07:04 lukepass

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

emulienfou avatar Apr 01 '19 20:04 emulienfou

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

danielrestrepo avatar Apr 02 '19 13:04 danielrestrepo

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.

emulienfou avatar Apr 02 '19 14:04 emulienfou

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?

danielrestrepo avatar Apr 02 '19 20:04 danielrestrepo

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

emulienfou avatar Apr 02 '19 20:04 emulienfou

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

danielrestrepo avatar Apr 02 '19 21:04 danielrestrepo

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

emulienfou avatar Apr 03 '19 14:04 emulienfou

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

danielrestrepo avatar Apr 04 '19 13:04 danielrestrepo

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 avatar Jun 05 '19 03:06 aquibbaig

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

aquibbaig avatar Jun 08 '19 08:06 aquibbaig

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?

marcin-dorosiewicz avatar Nov 27 '19 15:11 marcin-dorosiewicz

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!

aquibbaig avatar Nov 27 '19 17:11 aquibbaig