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

FEATURE: Support multiple Prototype Generator #2070

Closed

Conversation

dfeyer
Copy link
Contributor

@dfeyer dfeyer commented May 23, 2018

This feature allow to register multiple Fusion Prototype Generator, by using the same syntax we use for the super types definition. Like this a Package can introduce an prototype generator, without disabling silently the existing one. It's backward compatible with the current configuration.

The configuration is adapted like this:

Neos.Neos:Content:
  options:
    fusion:
      prototypeGenerator:
        'Medialib\Fusion\PrototypeGenerator\ContentPrototypeGenerator': true
        'Ttree\Writer\ContentPrototypeGenerator': true

Todos:

  • Documentation update
  • Schema update
  • Throw an exception if the configuration is broken

@dfeyer dfeyer added the I: WIP label May 23, 2018
@dfeyer
Copy link
Contributor Author

dfeyer commented May 23, 2018

I'm intersted to have more feedback on this feature, before adapting documentation and doing some code cleanup.

@dimaip
Copy link
Contributor

dimaip commented May 23, 2018

Makes sense 👍
But in general I start to loathe prototype generators more and more over time...

@mficzel
Copy link
Member

mficzel commented May 23, 2018

/o\ Please do not add this. With multiple generators the amount of invisible fusion code integrators have to deal with gets even bigger. I would rather get rid of the whole feature.

@dfeyer
Copy link
Contributor Author

dfeyer commented May 23, 2018

/o\ Please do not add this. With multiple generators the amount of invisible fusion code integrators have to deal with gets even bigger. I would rather get rid of the whole feature.

A bit more background on my use case, I work on more powerful content editing mode. In this case a generator make sense, if the user need to customize the generated code, it will not require to unset the object, just adding some path in the generated one. Unsetting the object if awkward, I agree. But code generation can be really useful sometimes.

@mficzel
Copy link
Member

mficzel commented May 23, 2018

@dfeyer The problem i see is that such features will be used in the wrong places a lot. The smart developers that will use such features to be more efficient often have a hard time understanding that they are making the project way harder to understand for other people.

A little code repetition is a cheap price if the projects can be understood more easily.

BTW: Why do'nt you create a derived prototype generator for your use-case.

@dfeyer
Copy link
Contributor Author

dfeyer commented May 23, 2018

BTW: Why do'nt you create a derived prototype generator for your use-case.

I can go this way, no problem. I agree that this is a low level / tricky extension point but the current situation is fragile. As soon as you introduce a Generator some strange thinks can happen, because you replace the existing one.

@mficzel
Copy link
Member

mficzel commented May 24, 2018

As soon as you introduce a Generator some strange thinks can happen, because you replace the existing one.

If you are implementing your own generator you should be absolutely aware of the previously existing ones you interfere with. Otherwise strange things will happen anyways.

@dfeyer
Copy link
Contributor Author

dfeyer commented Jun 7, 2018

I'm intersted to have more comments on this one. My main motivation is to use this the generate prototype that are attached to a specific Editing mode, and this need to be pluggable, so a new package, will add new prototype for preview mode A, an other package will add one for preview mode B, ...

If you are implementing your own generator you should be absolutely aware of the previously existing ones you interfere with. Otherwise strange things will happen anyways.

I agree with your concern, this is a dangerously magic/internal extension point. But it's powerfull, and the current solution is really limited.

@mficzel
Copy link
Member

mficzel commented Jun 7, 2018

For the record, my concerns are not meant as any kind of veto. If you consider them and decide this is fine i am totally fine with your decision. That is why i only placed a comment and no negative review.

@jonnitto
Copy link
Member

I know what you are working on (content editing mode), so I think this is a nice feature to have for package developers. And I think Neos needs a bit of more shiny packages like that…

@kitsunet
Copy link
Member

kitsunet commented Jun 21, 2018

I am rather divided by this. On the one hand I can see usecases, on the other I also think prototpe generators make it rather hard to reason about a project.

BUT IMHO Neos was always about the possibility. I don't want to restrict people to the one and only way (that solves only one usecase), been there done that, not helpful. People will then find workarounds that are even harder to reason about. In the end you want to have clear paths for differnet usecases and hope that users choose the right path for the right usecase (tutorials, exmaples, docs can help with that).

I would rather only want to prevent certain paths if it is clear that those are really harmful on a technical or conceptual level. But IMHO this is not, just because in some usecases doing everything (or most things) with generators is just what you need.

I the end rather a pro than a con. Although I think this could be solved with a CompoundGenerator that just accepts a config of downstream generators without changing the config format and also still keeping the entry point singular as a little restriction to the generator feature.

@dfeyer
Copy link
Contributor Author

dfeyer commented Aug 8, 2018

I did some experiment with a custom CompoundGenerator, works fine, but sounds weird that any community package that require a custom generator can fully break my configuration.

<?php
declare(strict_types=1);

namespace Ttree\Fusion\CompoundGenerator;

use Neos\ContentRepository\Domain\Model\NodeType;
use Neos\Neos\Domain\Service\DefaultPrototypeGeneratorInterface;
use Neos\Flow\Annotations as Flow;

class CompoundGenerator implements DefaultPrototypeGeneratorInterface
{
    /**
     * @var array
     * @Flow\InjectConfiguration(package="Ttree.Fusion.CompoundGenerator")
     */
    protected $settings = [];

    public function generate(NodeType $nodeType)
    {
        $generators = $this->buildGeneratorList($nodeType);
        foreach ($generators as $generator) {
            /** @var DefaultPrototypeGeneratorInterface $generator */
            $generator = new $generator();
            $generator->generate($nodeType);
        }
    }

    protected function buildGeneratorList(NodeType $nodeType, array $generators = []) {
        /** @var NodeType $superType */
        foreach ($nodeType->getDeclaredSuperTypes() as $superType) {
            if (isset($this->settings[$superType->getName()])) {
                $generators = array_merge($generators, array_keys(array_filter($this->settings[$superType->getName()])));
                $generators = $this->buildGeneratorList($superType, $generators);
            }
        }
        return $generators;
    }
}

@jonnitto
Copy link
Member

From my point of view, I would like to see this feature in Neos 4.2. Can we come to a decision about this?

Copy link
Member

@mficzel mficzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine, by reading.

As i said above my comment was never intended to be blocker.
If others need this we should support this.

@jonnitto
Copy link
Member

@dfeyer Do you need help to finish this?

@kitsunet
Copy link
Member

We need to update schema and documentation for this to go in, but shouldn't be too much work.

@jonnitto
Copy link
Member

ping @kitsunet @dfeyer

@jonnitto
Copy link
Member

Can we finish this PR? I would like to include this into the next release…

@jonnitto
Copy link
Member

Last call for this PR to go into the next version @dfeyer @kitsunet

@stale
Copy link

stale bot commented May 30, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions!

@stale stale bot added the Stale Issues and PRs with no recent activity, about to be closed soon. label May 30, 2019
@stale
Copy link

stale bot commented Jun 13, 2019

This issue has been automatically closed because it has not had activity for some time. But that does not need to be final. If you find the time to work on it, feel free to open it again. Thanks again for your contributions!

@stale stale bot closed this Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: WIP Stale Issues and PRs with no recent activity, about to be closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants