[WIP] Support for inherited classes #815

Merged
merged 1 commit into from Jul 9, 2012

Conversation

Projects
None yet
3 participants
Contributor

michelsalib commented Jun 9, 2012

@rande like I said yesterday, I cleaned my code in order to submit a 'clean 'proposal.

You just have to extend InheritanceAdmin instead of Admin and the declare your service as follow:

<service id="bcc_test.admin.base" class="BCC\TestBundle\Admin\BaseAdmin">
    <tag name="sonata.admin" manager_type="orm" group="bcc" label="base"/>
    <argument/>
    <argument>BCC\TestBundle\Entity\Base</argument>
    <argument/>
    <argument type="collection">
        <argument key="student">BCC\TestBundle\Entity\Student</argument>
        <argument key="teacher">BCC\TestBundle\Entity\Teacher</argument>
    </argument>
</service>

The only thing that changes is the last argument which is the list of supported subclasses, keys are used to render label (for the moment).

In order to support different forms, you can simply do:

<?php
protected function configureFormFields(FormMapper $form)
{
        $subject = $this->getSubject();

        if ($subject instanceof Teacher) {
            $form->add('course', 'text');
        }
        elseif ($subject instanceof Student) {
            $form->add('year', 'integer');
        }

        $form->add('name');
}

I want to have some feedback before cleaning and documenting the code.

  • Things are a bit hackish for the moment, do we need to refactor the Admin class in order to have a cleaner override (because this one duplicates code and forgets about the ModelManager).
  • Currently the subclasses are given trough configuration, what do you think about an abstract method in the InheritanceAdmin that asks for the subclasses? It will be helpful to specify better label or other behavior later. The Doctrine Orm Admin bundle could also provide a default implementation later from the discriminator map.
Contributor

michelsalib commented Jun 10, 2012

@rande what do you think about it ?

Admin/InheritanceAdmin.php
+{
+ protected $subClasses;
+
+ public function __construct($code, $class, $baseControllerName, $subClasses)
@rande

rande Jun 11, 2012

Owner

Can you add the phpdoc ?

@michelsalib

michelsalib Jun 11, 2012

Contributor

I will document once the PR is ready ;)

Admin/InheritanceAdmin.php
+ $this->subClasses = $subClasses;
+ }
+
+ public function getNewInstance()
@rande

rande Jun 11, 2012

Owner

the getClass should be return the correct class depends on the request parameter. This shoudl also check if the request is set.

@michelsalib

michelsalib Jun 11, 2012

Contributor

The matter is, the getClass function is use for other purposes (like menu generation if I remember well). Changing the behavior of this function will lead to too much refactoring IMO.

@rande

rande Jun 11, 2012

Owner

You are right, let's see the complete implementation now ;)

@michelsalib

michelsalib Jun 11, 2012

Contributor

ok, I will dedicate some time later today.

Admin/InheritanceAdmin.php
+
+namespace Sonata\AdminBundle\Admin;
+
+abstract class InheritanceAdmin extends Admin
@rande

rande Jun 11, 2012

Owner

I am not sure this class is needed. We can do all the job inside the main Admin class.

@michelsalib

michelsalib Jun 11, 2012

Contributor

Ok, I will merge the two classes. Nice catch!

Contributor

michelsalib commented Jun 11, 2012

@rande I just pushed an update. The implementation is cleaner now, thanks for the tip :)
When I have your go, I will document the code and the doc. I also have some filters that I would like to add in the orm admin bundle, and some special columns too.

Admin/Admin.php
*/
- public function __construct($code, $class, $baseControllerName)
+ public function __construct($code, $class, $baseControllerName, array $subClasses = array())
@rande

rande Jun 11, 2012

Owner

This is optional, so it need to be injected through setter

Admin/Admin.php
+ $class = null;
+
+ if ($this->request) {
+ $class = $this->getRequest()->get('subclass');
@rande

rande Jun 11, 2012

Owner

subclass must contains a key and not the real class name.

Admin/Admin.php
@@ -1082,7 +1102,7 @@ public function getFormBuilder()
}
)));
- $this->formOptions['data_class'] = $this->getClass();
+ $this->formOptions['data_class'] = $this->getRequest()->get('subclass', $this->getClass());
@rande

rande Jun 11, 2012

Owner

you need to check if the Request is set

Contributor

michelsalib commented Jun 11, 2012

@rande All fixed, I also added php doc blocks.
I also started to look into the documentation, but it seems de-synchronized from the sonata website...

Contributor

michelsalib commented Jun 11, 2012

I just squashed ;)

Admin/Admin.php
+ $class = null;
+
+ if ($this->request &&
+ null !== ($subclass = $this->getRequest()->get('subclass')) &&
@rande

rande Jun 11, 2012

Owner

can you create a specific function ? and throw an exception if subclass is set but the key does not exist ?

Contributor

michelsalib commented Jun 11, 2012

Et voilà ! ;)

Exception/UndefinedSubClassException.php
+
+namespace Sonata\AdminBundle\Exception;
+
+class UndefinedSubClassException extends \Exception
@rande

rande Jun 11, 2012

Owner

This should extends RuntimeException

Admin/Admin.php
+ return $this->subClasses[$subclass];
+ }
+ else {
+ throw new UndefinedSubClassException('Cannot find subclass with corresponding index "'.$subclass.'"');
@rande

rande Jun 11, 2012

Owner

Can you use sprintf for string concatenation ?

@stof

stof Jun 11, 2012

Contributor

throwing an exception here means that the client of your site is able to break things by passing a GET param.

Admin/Admin.php
+ * @return string|null
+ * @throws UndefinedSubClassException
+ */
+ private function getSubClass()
@rande

rande Jun 11, 2012

Owner

After a second though, I think the getSubClass should accept a $name argument. So you need to add a few more methods :

  • hasSubClass method
  • hasActiveSubClass method
  • getActiveSubClass method
Owner

rande commented Jun 11, 2012

@michelsalib the add action is also defined in different places : edit form, show and on the dashboard block. So this part is still missing to create a valid object.

Admin/Admin.php
+ if (isset($this->subClasses[$subclass])) {
+ return $this->subClasses[$subclass];
+ }
+ else {
@stof

stof Jun 11, 2012

Contributor

no need to use else as the if returns (and btw, the else is misplaced)

Admin/Admin.php
@@ -1051,7 +1100,7 @@ public function getTemplate($name)
*/
public function getNewInstance()
{
- return $this->getModelManager()->getModelInstance($this->getClass());
+ return $this->getModelManager()->getModelInstance($this->getSubClass()?:$this->getClass());
@stof

stof Jun 11, 2012

Contributor

you should add spaces around the ternary operator: $this->getSubClass() ?: $this->getClass()

Contributor

michelsalib commented Jun 12, 2012

I just fixed all your remarks (thanks @stof and @rande).
I hope the template factorization is ok. I did not included the dashboard block because the message and the image are different.
I also removed the exception because of @stof point. I just did so in order to move forward, we can of course discuss it.

Contributor

michelsalib commented Jun 19, 2012

@rande what can I do to move this release forward ?

Owner

rande commented Jul 3, 2012

Can you rebase this PR and add some documentations ?

Contributor

michelsalib commented Jul 3, 2012

Sure, will do tomorrow ;)

[WIP] Add Inheritance admin to support CRUD on inherited classes
Tweak

Refactor sub class access logic
- factorize create button (with some css fixes)
- remove unnecessary exception

Add doc, and fix some css
Contributor

michelsalib commented Jul 4, 2012

Hi @rande I rebased/squash. I also added some doc ;)

rande added a commit that referenced this pull request Jul 9, 2012

Merge pull request #815 from michelsalib/inheritance
[WIP] Support for inherited classes

@rande rande merged commit 92a84cf into sonata-project:master Jul 9, 2012

Contributor

michelsalib commented Jul 9, 2012

Thanks @rande. I want to submit a PR with special type filter (for the filtering form) and column (for the datagrid) in the next days. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment