BC break for the display in menu logic

dbu commented 10 years ago

In 1.0, whether a page should be shown in the menu (getOptions()['display']) was controlled based on whether the label is set: https://github.com/symfony-cmf/SimpleCmsBundle/blob/1.0/Model/Page.php#L350

For 1.1 we made it possible to configure the display field explicitly. The new logic is simply 'display' => $this->display,. This is not ideal, as we start displaying Pages that have an empty label. There are a couple of options:

Leave as is and let people live with it. Its not very user friendly however.
Default display to false instead of true, and set it to true automatically when setting the label. Could have unexpected side effects however.
Change the logic in getOptions to 'display' => $this->display && !empty($this->label),
Default display to null. But with the form layer, this won't work as expected as a boolean field will always set the display option to a boolean.

benglass commented 10 years ago

I would vote to make the non-user friendly BC and leave it as it is because that seems the most logical setting. Using the label to determine whether the node should be shown in menu seems like it was a quick and dirty solution.

dbu commented 10 years ago

With non-user friendly i do not just mean that its a BC. The problem is that now when i do not specify a label, i get a dead empty menu entry. And if my dev who understands this then went and set display to false (once we support this), i might enter that label and am again confused why the menu entry does not appear. Somehow we would need some sort of true|false|auto here and by default keep auto... Maybe we could do a doctrine callback method on the object at least on prePersist, to sanitize. Or even store whether we changed display / label and when label was added set display true, when label was set to empty, set display false.

benglass commented 10 years ago

Could this could be resolved by instructing people just setting display to false for any menu nodes that have no label when they upgrade?

On Sat, May 10, 2014 at 1:01 PM, David Buchmann notifications@github.comwrote:

With non-user friendly i do not just mean that its a BC. The problem is that now when i do not specify a label, i get a dead empty menu entry. And if my dev who understands this then went and set display to false (once we support this), i might enter that label and am again confused why the menu entry does not appear. Somehow we would need some sort of true|false|auto here and by default keep auto... Maybe we could do a doctrine callback method on the object at least on prePersist, to sanitize. Or even store whether we changed display / label and when label was added set display true, when label was set to empty, set display false.

— Reply to this email directly or view it on GitHubhttps://github.com/symfony-cmf/SimpleCmsBundle/issues/112#issuecomment-42747584 .

benglass commented 10 years ago

Another thought, this seems like a bit of a validation issue but if you have display set to true then label should be required which would resolve the issue going forward.

We could provide a few lines of example script that would pull down your Page docs and set display to false if label was empty.

Presuming there is validation that makes label required if display is not false then there would be no need to have any knowledge of label in the determination of the value of display. This seems like it would be the expected behavior for newcomes and for people using 1.0 they would fix their page docs so if they were relying on implicit display false due to empty label that it becomes explicit.

Just my 2 cents and how we handle it in our CMS.

On Sat, May 10, 2014 at 3:10 PM, Ben Glassman bglassman@gmail.com wrote:

Could this could be resolved by instructing people just setting display to false for any menu nodes that have no label when they upgrade?

On Sat, May 10, 2014 at 1:01 PM, David Buchmann notifications@github.comwrote:

With non-user friendly i do not just mean that its a BC. The problem is that now when i do not specify a label, i get a dead empty menu entry. And if my dev who understands this then went and set display to false (once we support this), i might enter that label and am again confused why the menu entry does not appear. Somehow we would need some sort of true|false|auto here and by default keep auto... Maybe we could do a doctrine callback method on the object at least on prePersist, to sanitize. Or even store whether we changed display / label and when label was added set display true, when label was set to empty, set display false.

— Reply to this email directly or view it on GitHubhttps://github.com/symfony-cmf/SimpleCmsBundle/issues/112#issuecomment-42747584 .

dbu commented 10 years ago

if we keep it as is, we should indeed provide a migration script. but i am more worried of the user interface:

1. i add a label. 2. i save. 3. i don't see my menu entry.
1. i delete the label. 2. i try to save. 3. it complains i also need to uncheck display.

both feel unfortunate to me. if we had a callback on the document, we could track if the label changed from empty to provided or the other way round, and in those cases automatically update display. the only real usecase for the display field i see is to hide something from the menu even though it has a label set. or maybe show something even though it has an empty label, using a custom menu renderer (or javascript black magic). even with the listener, that would still be possible to achieve.

lsmith77 commented 10 years ago

ping

dbu commented 10 years ago

i guess the migration issue is kind of history by now...

if we agree its a good idea, i can do a PR to automatically set display to false when the label is set to empty, and display to true if the label changes from empty to something. i guess we should do that in a preUpdate event callback, to not overwrite eventual manual changes to the display value.

having a validation to not set display to true when label is empty would be nice, but doing that would make the first point of the automatic update never happen because the validation fails. we would need to handle this by javascript in the frontend, or in the form layer, as the two fields label and display just start being coupled.

dbu commented 10 years ago

opinions on automatically handle the display setting when possible?

lsmith77 commented 10 years ago

a preUpdate listener sounds sensible ..

lsmith77 commented 10 years ago

though could we not also do this on read in the provider?

dbu commented 10 years ago

thanks for the hint lukas. turned out we can be even simpler than that. see #124

dbu commented 10 years ago

fixed in #124

symfony-cmf / simple-cms-bundle

BC break for the display in menu logic #112