[WIP] Refactored allow empty

[dantleech]

I don't like declaring options in the constructor, its not very flexible. Have changed to a method call and also fixed the test.

Also, I don't understand this logic:

if (!$this->allowEmptyItems) {
    return null
}

Surely that should be if $this->allowEmptyItems === true return null?. Throwing an exception would seem to indicate that empty items are not allowed. Also, can we be more concise with the naming - what are we allowing here? What is an empty item? The logic would seem to indicate "allowRouteNotFound" - but is maybe more acurate if we refactor it to "allowEmptyContent" ?

Just my thoughts.

[dantleech]

/cc @rivaros

[rivaros]

returning null means 'skip this menuitem'
therefore if allow_empty_items=false, we skip them (default behaviour), otherwise we proceed and create them.

[dantleech]

@rivaros ok thanks, I get that now.

[sjopet]

I opt for having link types really;

• internal (document relation)
• external (uri)
• no link

then you can couple the different behaviors to a class or constant at least.
I don't want to interfere with this pr just giving my view I guess we ca talk about it next week.

[dantleech]

sjopet: what would you propose for an API?

[sjopet]

this is what we currently have: https://github.com/sjopet/cmf-stuff I just noticed my colleague added some comments in the interface that describe the purpose.
For the form we use a custom formtype so you can choose the type with a select field and some js to show and hide the corresponding fields.

keep in mind the BaseMenuItem which is extended, is a very old version.

[rivaros]

Hello,
@sjopet, is this all about possibility to have different types of menuitems (internal, external, link etc) in one sonata list but using differrent forms for editing them?

[sjopet]

No there's only one type of menuItem but it can link to different things.

• An external uri eg: http://google.com
• An internal link to a document /cmf/sandbox/content/some_document
• or no link: then the item is used just to give some structure to the menu like a column or dropdown.

The form type I mentioned in the previous comment is just for adding the link to the menu item. So if you choose internal from the type selection dropdown you would see a tree selector to select the document to link to. if you select external you should see a a simpel text input field and when 'no link' is selected you wo'nt see any fields.

[dbu]

$this->assertNull()?

[dbu]

i like the refactoring you propose here. can you rebase on latest master and maybe have an assertion in the test, then lets merge!

@sjopet regarding the form type for editing menu items: sounds like a useful thing. glad if you can PR that into the admin (note that there is currently another PR by @dantleech introducing menu roots as specific document types - just to avoid conflicts)

[dantleech]

@sjopet yes - this is exactly what I want to implement too. I.e. I want the "route target" to be a radio selection in the UI - and for that we need to store the linkType. This would also remove any confusion about "precedence" as it is explict in addition to allowing the document to remember link-type values (i.e. they can change from external URI to Content and then back again without having to remember the URI).

I was going to have a look at this quite soon, maybe today.

[dbu]

well the linkType could be implicit by only having one of the 3 ever set at the same time. storing linkType explicitly is redundant, and its problematic if people add their own additional things. i would prefer to make this a UI thing and just document the precedence in case people use an inferior UI that allows to generate potentially ambigous situations.

[dantleech]

I think the main benefit of a linkType is that the user doesn't have to erase data to change the target. Say if I have an external URL: http://www.example.com/somecontent, and I want to temporarily forward to a holding page, which is a reference to an internal content object content1, then I would have to delete the URL (or set the content to null depending on the precedence).

imo the user should be able to explicitly choose between multiple target/link types, and the values of these types should not be dependant on which one is currently "in use".

Also, incase I didn't mention it, I want to improve the UI by making the target selection a RADIO select, and I think adding the linkType is the only thing that really works.

[sjopet]

I have to agree with dan here. I don't think this has to give problems with a different UI if you make the linkType a mandatory field or give it a default value.
Also would it not be more clear having a dedicated variable for the type of link that has precedence when people add additional things and thus add logic. In the current situation you have a bunch of logic to determine the correct link which makes customizing the menu stuff harder.

[dbu]

alright guys, you win :-)
for the migration, we will have to write a custom script that follows our current order and creates that property. don't think this is sensibly doable with phpcr:node:update

[dantleech]

Regarding the migration, I have just made a PR for the nodes:update command for that closure thing I mentioned. Personally I would find it very useful, and it would make a great copy-paste migration solution for things like this. But I don't want to rush it through as I think it is potentially Evil :)

phpcr/phpcr-utils#71

[dantleech]

ok. I have fixed the test, granted its a bit of an obfuscated one, but will hopefully revisit this with refactoring discussed here. Good to merge for me.