search

symfony2admingenerator / GeneratorBundle

Admingenerator for Symfony. Parse YAML files to build customized backend.
MIT License
67 stars 29 forks source link

Feature lock - discuss features for next major version #38

Open ioleo opened 9 years ago

ioleo commented 9 years ago

So.. another year has almost passed. I really would like to start locking features, focus on fixing bugs and document everything. It would be nice if we could ship the next major version (and the first really stable) at the beggining of 2015.

What needs to be done?

Before we start documenting everything, there are some areas that are poorly implemented, and some features that either don't work... or work partially. Examples?

NestedList

It was designed as a simple tree-structure display, but neither scopes nor filters have been implemented on Nested List. It's not well documented and it was styled back in the twitter bootstrap2 time.. and the templates have not been reviewed or tested since. I'm not even sure anyone except me ever used this? I'm not sure it still works?

We need to either (a) fix nested list, or (b) remove it. We can't have a half-working, untested feature.

Form field options

You can define field options in the YAML file, but some things are simply not possible in YAML. For example - you don't have access to the current security token (or any other service). Ofcourse, you can add/modify the options in the generated FormType class. I actually moved ALL my formOptions to the classes, just to make the generator.yml file more readable.

Lately, in the new-unstable repository, I've introduced some modifications to formType class to make it quick and easy see.

Simply.. if you create in YourVendor/YourBundle/Form/Type/YourModel an Options.php class with getYourFieldnameOptions method - it will be used for all NewType, EditType and FiltersType.

However, if you add getYourFieldnameOptions method to any of these formType classes - it will take precedence over the "common" Options class.

For example, you can add getStatusOptions() { return array('foo' => 'bar'); } to Options class and getStatusOptions() { return array('foo' => 'baz'); } to EditType class. The 'foo' => 'bar' will be used for NewType and FilterType, and the 'foo' => 'baz' will be used for EditType.

Options class and formType classes are automatically injected with securityContext service. Another tweak is passing down validation_groups for embeded collections.

I feel like useing YAML to add form options is obsolete and maybe we should remove that feature? Or at least document that the recommended way is to use Options/FormType classes to handle form options.

Generic/object/batch actions

This has been a pain in the ass. Some actions require global (common) configuration for all builders, while some shouw be reconfigured (or removed) for certain builders. We've addressed this issue with this method however, I'm still not satisfied.

I feel like we really need to change the implementation.

Propel support? Doctrine ODM support?

It's great to have all 3 ORMs on board... but are all the features working for all ORMs? I'm not sure about nested list and a few others.

More?

What we really need right now, is to sit back for a moment and think: which of these features we really need? I think it's time to discuss a feature lock and decide what GeneratorBundle will be, and what it will not be.

ioleo commented 9 years ago

Example generator... a lot of repeating with actions :/

generator: admingenerator.generator.doctrine
params:
    # here fields, scopes, tabs definition
    actions:
        new:                { credentials: "someTest()" }
    object_actions:
        print:
            label:          "object_actions.printMyModel.label"
            credentials:    "canPrintMyModel(object)"
            icon:           fa-print
            route:          AcmeDemoBundle_MyModel_print
            params:         { id: '{{ MyModel.id }}' }
        proposed:
            credentials:    "canProposeMyModel(object)"
            icon:           fa-send-o
            route:          inject_object_defaults
        accepted:
            credentials:    "canAcceptMyModel(object)"
            icon:           fa-check
            route:          inject_object_defaults
        sent:  
            credentials:    "canSendMyModel(object)"
            icon:           fa-check
            route:          inject_object_defaults
        received:
            credentials:    "canReceiveMyModel(object)"
            icon:           fa-quote-left
            route:          inject_object_defaults
        confirmed:
            credentials:    "canConfirmMyModel(object)"
            icon:           fa-paperclip
            route:          inject_object_defaults
        paid:
            credentials:    "canPayMyModel(object)"
            icon:           fa-check
            route:          inject_object_defaults
        show:   { credentials: "canShowMyModel(object)" }
        edit:   { credentials: "canEditMyModel(object)" }
        delete: { credentials: "canDeleteMyModel(object)" }
    batch_actions:
        revert:
            credentials:    "canBatchRevertMyModel()"
            icon:           fa-step-backward
            route:          inject_batch_defaults
builders:
    list:
            actions:    { new: ~ }
            object_actions: ~
            batch_actions: ~
    new:
        params:
            actions:    { save: ~, list: ~ }
    edit:
        params:
            actions:    { save: ~, list: ~ }
            object_actions:
            print: ~
            proposed: ~
            accepted: ~
            sent: ~
            received: ~
            confirmed: ~
            paid: ~
                show: ~
                delete: ~
    show:
        params:
            credentials: "canShowMyModel(object)"
            # title:      See custom template
            display: ~
            actions:    { list: ~, new: ~ }
            object_actions:
            print: ~
            proposed: ~
            accepted: ~
            sent: ~
            received: ~
            confirmed: ~
            paid: ~
                edit: ~
                delete: ~
    actions:
        params:
            object_actions:
            print: ~
            proposed: ~
            accepted: ~
            sent: ~
            received: ~
            confirmed: ~
            paid: ~
                delete: ~
            batch_actions:
                revert: ~
sescandell commented 9 years ago

:+1:

I'll comment soon. But good idea: we need a roadmap to know what v1.0 should be

ioleo commented 9 years ago

I think the "add mink/behat acceptance tests" #5 can be closed, as something that we don't actually need and implementing generalized acceptance tests that would work and be meaningfull.. would be hard.

It's better if we add some acceptance tests to a "demo bundle" and test each stable release against it (before tagging it).

sescandell commented 9 years ago

Hi there,

I'm not even sure anyone except me ever used this?

Hey, I am using it too !! :+1: (to manage a categories tree)

Nested List

Check my comment on #42 , I think we can, for a v1, target something simple, but working well. Maybe not implementing scopes neither filters. Just displaying tree... displaying well and allowing to add childs, sorting it simply. And thats' all (for a v1).

Form field options

What you did is very good... It was pretty boring to copy/paste options from a form type to another (especially between New and Edit ones). I also agree with you about formOptions in YAML files: when there is too many customisation, it could make the file difficult to read. But, I also really like this functionality for simple configurations (type option for collections, required option, simple options customizations). PHP are fine for deeper tuning (typically QueryBuilders : I never use the Twig Extension for that... more generally speaking, I never use PHP tricks in YAML files). So, for me, :+1: for your Options class and uniformising NewTypeand EditType options. But, I think YAML configuration for formOptions is still very usefull in global configuration. So, it's more like a :-1: to advice users not using it (but this is just MY opinion).

Generic/object/batch actions

Agree with you... I don't know if Symfony Configuration Component could help us or not for this... We should give it a try. I think this is very structuring. And we son need to "take a decision" before making the v1.

Propel support? Doctrine ODM support?

I'm only testing over DoctrineORM... (but also on SQLServer :) )

More?

introduce AdminLTE theme

I'm going to work on this

move templates to seperate bundle

Link to the previous point. Just an additional question about that: does Actions templates must be moved too?

fix excel builder (eg. empty fields or related fields)

I think you already fixed it, am I wrong? :)

ioleo commented 9 years ago

@sescandell OK. let's continue the nested list discussion in issue #42. And it's nice to hear my code was useful :D

About the formOptions: we could inform in docs about both ways and state something like "for simple options like... (here your examples) it may be easier/quicker to define them in YAML... however for more advanced options you may want to move them to the options/form class, to keep the generator.yml readable.

Just an idea that crossed my mind right now. Maybe we add new entry in global params.serives "editForm / newForm / filterForm / optionsClass"? If they are null (default) everything works the same as now:

But if they are set:

This would allow injecting any service (not only security) into the generated classes.

About config component -> I think this will be the next issue I will work on.

ioleo commented 9 years ago

NestedList discussion summary

During the discussion two diffrent ideas emerged of what NestedList should be:

1) Classic "view" list

This idea sees "list" as a view listing all items (and "nested list" is no diffrent) with additional features allowing to quickly find the item we're interested in:

The "editing" is out of scope of this definition, thus "drag & drop" to move node in the tree structure is not in the features list (and should be handled by a seperate action.. eg. in "edit" form OR through custom action in actions builder).

2) Special "view" list

This idea sees "nested list" as a special case which needs to be handled diffrently. As nested list deals with tree-structured data (which is used for various, well-defined purposes), the features of the list must fit the purpose of such data structure, and so we're interested in:

The idea of "pagination" breaks the tree structure, and as such, breaks the purpose of this list (eg. if the list was paginated, you could not "move" the node into "page 2", as it would not be displayed). We're sending the whole tree to the client ALWAYS. The filters and sorting should be handled on client-side by Javascript.

ioleo commented 9 years ago

@sescandell @ksn135 @neoshadybeat (and anyone else interested)

Please cast your vote, which solution you think we should go.

I vote for (2) as it seems better fit for tree-data purpose and will be easier to implement (and maintain).

ksn135 commented 9 years ago

@loostro _I vote for (1)._ But both options are very promising.

We're sending the whole tree to the client ALWAYS.

Just imagine if you need to show on client side (in second option) several hundreds items with many categories and many columns. Page load time will be too long.

ioleo commented 9 years ago

@ksn135

Just imagine if you need to show on client side (in second option) several hundreds items with many categories and many columns. Page load time will be too long.

I am aware of that.. but I believe it's impossible to create a solution to fit all use cases. In such situation, the developer would have to modify the default bevariour by himself (which he could do easily, since everything can be overwritten).

ksn135 commented 9 years ago

@loostro Well, yes, you are right. But we talking about some kind of "defaults" thats must be "suits mostly for all". And the second option ("with whole tree") for huge datasets is unacceptable, IMHO. At least make "whole tree" with nodes ONLY and get additional info via AJAX. Then I can live with it... )

ioleo commented 9 years ago

If we don't add the whole tree tran filtering on client side is impossible.

Also , if the drag&drop move is our priority, then we need the whole tree otherwise that feature would not be functional.

ksn135 commented 9 years ago

Probably we have different meaning of term "whole tree". _Option1:_ all (whole) data to display and manipulate tree in browser via javascript without additional AJAX request. _Option2:_ minimum amount of data sufficient to display and manipulate tree in browser via javascript WITH additional AJAX request (to get columns data or children not loaded on first page load) I mean Option2 is better then Option1. And Option1 is NOT acceptable solution for HUGE datasets.

ioleo commented 9 years ago

Very well. To everyone: please consider option1 and option2 prezentem by ksi and share your opinion

ioleo commented 9 years ago

Damn this mobile dictionary :/

sescandell commented 9 years ago

Hi,

As @loostro said, I think we should basically handle "most of the cases" (which is not hundred an hundred of nodes and leafs). Even in a "big blog project", I'm not sure you will have thousand and thousand rows in a "tree mode" (maybe I'm wrong)... And so option 2 sounds good (just wondering @loostro how do you imagine to manage "filters"? even on client side, what should be the filters behavior regarding nodes and leafs?)

IMHO, the easiest way to manage nested list is option 1: managing it like basic list (and doing what Sonata or Wordpress do for example).

So I would say:

Not sure this helps you :/

sescandell commented 9 years ago

I updated initial comment regarding "more" points (so we can easily get decisions of all discussions)

ioleo commented 9 years ago

Idea: As @sescandell suggested on gitter -> use CS Fixer in dev mode, to make generated code prettier

ioleo commented 9 years ago

@sescandell After configureing scrutinizer and travis I see some alarming results... our code coverage is poor (12%). Many classes contain long and complex methods. It would be good if (after finishing 2.0 features) we spent some time to improve the code quality.

sescandell commented 9 years ago

Agree.

Even if, for me, code coverage is not representative of code quality, there is two big things to do after features lock (IMO):

  1. Documentation
  2. Tests