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: Configurable Fusion node type prototype generators #767

Merged

Conversation

mficzel
Copy link
Member

@mficzel mficzel commented Sep 21, 2016

  • If fusion.prototypeGenerator is set to a className the class has to implement the interface
    \TYPO3\Neos\Domain\Service\DefaultPrototypeGeneratorInterface and is used to generate the prototype-code
    for this node.
  • If fusion.prototypeGenerator is set to null no prototype is created for this type.

For nodeTypes based on TYPO3.Neos:Node, TYPO3.Neos:Document or TYPO3.Neos:Content a backwards compatible implementation is added to the configuration and a new prototype generator for TYPO3.Neos:Plugin is added.

@mention-bot
Copy link

@mficzel, thanks for your PR! By analyzing the annotation information on this pull request, we identified @dfeyer, @radmiraal and @kdambekalns to be potential reviewers

@kitsunet
Copy link
Member

I like. Would also allow us to add a plugin generator as that is always wrong :)

@mficzel
Copy link
Member Author

mficzel commented Sep 21, 2016

@kitsunet ... i'm impressed, you were almost faster then the mention bot.

What exactly should be added to the defaultPlugin Prototypes? I do'nt use it that often.

templatePath = 'resource://Vendor.Site/Private/Templates/NodeTypes/Content.SpecialNodeType.html'

# all properties of the nodeType are passed to the template
date = ${q(node).property('date)}
Copy link
Contributor

Choose a reason for hiding this comment

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

double space

Copy link
Member

Choose a reason for hiding this comment

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

..and missing closing single quote


By default Neos has generators for all nodes of type TYPO3.Neos:Node and creates protoypes based on
TYPO3.TypoScript:Template. A template path is assumed based on the package-prefix and the nodetype-name. All properties
of the node are passed to the template. For nodeTypes of type TYPO3.Neos:Document or TYPO3.Neos:Content the
Copy link
Contributor

@aertmann aertmann Sep 21, 2016

Choose a reason for hiding this comment

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

nodetype names should be quoted..

If ``options.fusionPrototypeGenerator`` is set to ``null`` no prototype is created for this type.

By default Neos has generators for all nodes of type TYPO3.Neos:Node and creates protoypes based on
TYPO3.TypoScript:Template. A template path is assumed based on the package-prefix and the nodetype-name. All properties
Copy link
Contributor

@aertmann aertmann Sep 21, 2016

Choose a reason for hiding this comment

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

nodetype names should be quoted with ``

@@ -2,6 +2,8 @@
'TYPO3.Neos:Node':
label: "${String.cropAtWord(String.trim(String.stripTags(String.pregReplace(q(node).property('title') || q(node).property('text') || ((I18n.translate(node.nodeType.label) || node.nodeType.name) + (node.autoCreated ? ' (' + node.name + ')' : '')), '/<br\\W*?\\/?>|\\x{00a0}|[[^:print:]]|\\s+/u', ' '))), 100, '...')}"
abstract: TRUE
options:
fusionPrototypeGenerator: TYPO3\Neos\Domain\Service\DefaultPrototypeGenerator
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm debating if the key options is a good idea here.. options is pretty broad and all of this configuration is somehow options.. would a key like fusion and then prototypeGenerator on next level make more sense?

@aertmann
Copy link
Contributor

btw. what to do about the existing interface overriding? that would stop working as expected right? if so this is a breaking change..

@aertmann
Copy link
Contributor

@skurfuerst @bwaidelich maybe you guys have input to this one (continued from thread)..

@bwaidelich
Copy link
Member

yes, I like @aertmann s idea to use "fusion" as entry point and we already do that for the Neos UI with "ui"

@mficzel mficzel force-pushed the feature/makeFusionPrototypeGeneratorConfigurable branch from fc62aad to 943cc42 Compare September 22, 2016 07:12
@mficzel
Copy link
Member Author

mficzel commented Sep 22, 2016

I adjusted the option name to fusion.prototypeGeneratorClassName, updated the documentation and added a Plugin Prototype Generator.

@aertmann this is somehow breaking but it will only go to 3.0 anyways.

@mficzel mficzel changed the title FEATURE: Add option options.fusionPrototypeGenerator for nodeTypes !!! FEATURE: Add option fusion.prototypeGeneratorClassName for nodeTypes Sep 22, 2016
@dimaip
Copy link
Contributor

dimaip commented Sep 22, 2016

@aertmann I think it would be better to make this breaking than to keep the two mechanisms. I doubt many people already use that feature anyways.

@dimaip
Copy link
Contributor

dimaip commented Sep 22, 2016

prototypeGeneratorClassName is very verbose though, any chances of making it shorter, e.g. prototypeGenerator? Wdyt?

@mficzel mficzel force-pushed the feature/makeFusionPrototypeGeneratorConfigurable branch from 943cc42 to 7cde216 Compare September 22, 2016 13:11
@mficzel
Copy link
Member Author

mficzel commented Sep 22, 2016

Renamed the option to fusion.prototypeGenerator

@mficzel mficzel changed the title !!! FEATURE: Add option fusion.prototypeGeneratorClassName for nodeTypes !!! FEATURE: Add option fusion.prototypeGenerator for nodeTypes Sep 22, 2016
@aertmann aertmann changed the title !!! FEATURE: Add option fusion.prototypeGenerator for nodeTypes !!! FEATURE: Configurable Fusion node type prototype generators Sep 22, 2016
@aertmann
Copy link
Contributor

looks great! nice one

@dimaip
Copy link
Contributor

dimaip commented Sep 30, 2016

Can we already merge this stuff into master now, or only after we branch off 3.0?
If it's possible to do it now, the PR needs to be rebased and I'll review it.

@aertmann
Copy link
Contributor

it's fine to merge breaking stuff for next major into master now

@johannessteu
Copy link
Contributor

Yeah, also branching will be done not soonish

@mficzel mficzel force-pushed the feature/makeFusionPrototypeGeneratorConfigurable branch from 7cde216 to 532d85c Compare September 30, 2016 16:13
@mficzel
Copy link
Member Author

mficzel commented Sep 30, 2016

I rebased the pr ... seems something broke by doing so. Will check the next days

@mficzel
Copy link
Member Author

mficzel commented Oct 2, 2016

I cannot reproduce the failing tests locally. Unit and functional tests are running fine. Also the error looks kind of unrelated to this change.

@mficzel
Copy link
Member Author

mficzel commented Oct 4, 2016

The errors were introduced in master with b2a1b0f and are not caused by this pr. I will rebase again as soon as the master is green again.

@mficzel
Copy link
Member Author

mficzel commented Oct 6, 2016

Finally the tests are green again ...

Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

The requested change makes a lot of small detail changes necessary in the NodeTypes configuration we have so far and in the generator, but I think it's for the better in terms of maintainability.

@@ -75,6 +75,10 @@ additionalProperties:
'options':
type: dictionary

'fusion':
Copy link
Member

Choose a reason for hiding this comment

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

I was almost about to merge this, but this IMHO cannot happen, it implicitly binds CR to Fusion, which it shouldn't be. SORRY :/

The configuration should go under "options" and not be mentioned in this schema.

Copy link
Member

Choose a reason for hiding this comment

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

I agree to your point but TBH I don't think it's a better solution to "hide" this defacto dependency within some option that is not part of the schema.
IMO a good solution would be: extensible schemas. That would allow the Neos or Fusion package to add this option to the NodeTypes schema. And we could use this for other parts, too.
It shouldn't be to hard to adjust the SchemaValidator accordingly. In fact, it might already be able to do that.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but I thought options was the current way to make this schema extensible?
So Neos can declare the fusion subkey in options and all currently possible subkeys of fusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

The key was renamed to options.fusion.prototypeGenerator as requested

@@ -119,6 +119,32 @@ The following options are allowed:
Alternatively the class of a node label generator implementing
``TYPO3\TYPO3CR\Domain\Model\NodeLabelGeneratorInterface`` can be specified as a nested option.

``fusion``
Copy link
Member

Choose a reason for hiding this comment

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

The below mentioned request to change the way the configuration is setup obviously makes a change here necessary.

@mficzel
Copy link
Member Author

mficzel commented Nov 7, 2016

Ranamed the configuration property to options.fusion.prototypeGenerator as requested by @kitsunet and rebased on current master ... no clue why travis complains on postgres now ... will look into this later

@kitsunet
Copy link
Member

kitsunet commented Nov 7, 2016

Thanks! Looks great. The Travis failure is not related, it's the timezone issue fixed by this PR (which is not yet upmerged) #1214

- If  ``fusion.prototypeGenerator`` is set to a className the class has to implement the interface
  ``\TYPO3\Neos\Domain\Service\DefaultPrototypeGeneratorInterface`` and is used to generate the prototype-code
   for this node.
- If ``fusion.prototypeGenerator`` is set to ``null`` no prototype is created for this type.

For nodeTypes based on ``TYPO3.Neos:Node``, ``TYPO3.Neos:Document`` or ``TYPO3.Neos:Content`` a backwards
compatible implementation is added to the configuration and a new prototype generator for ``TYPO3.Neos:Plugin`` is added.
… to ``options.fusion.prototypeGeneratorPath``
@mficzel mficzel force-pushed the feature/makeFusionPrototypeGeneratorConfigurable branch from 9222187 to af39048 Compare November 12, 2016 11:10
@mficzel
Copy link
Member Author

mficzel commented Nov 12, 2016

I rebased on current master to get rid of the ci-failures. No changes other than that.

@gerhard-boden gerhard-boden removed their assignment Nov 23, 2016
@mficzel mficzel merged commit e431b85 into neos:master Nov 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants