Skip to content

Conversation

@richardhj
Copy link
Member

@richardhj richardhj commented Aug 26, 2018

Issue

When creating a new item and finally saving the item, neither the alias attributes nor the combined values attributes get generated.

Description

The item gets initialized with $arrData = [] meaning we have to initialize all attributes so that they are present in the data array. This is related to #894 and 588eddf respectively.
When $arrData is empty, isAttributeSet() returns false which results in no attributes get notified about a model change.

Reproduction

  1. Using the API
$item = new Item($metaModel, null);
$item->set('name', 'Test');
$item->save();
// neither alias nor combined values have been generated now
  1. Using the Frontend Editing.
    Neither alias nor combined values are generated on creation.

  2. MetaModels 2.0 as well 2.1 are affected.

To be discussed

  • We might want to add a getEmptyValue() to the IAttribute.
  • We can move the new logic from the model to the getEmptyModel().

Checklist

  • Read and understood the CONTRIBUTING guidelines
  • Created tests, if possible
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself to the @authors in touched PHP files
  • Checked the changes with phpcq and introduced no new issues

@zonky2 zonky2 added the enhancement This issue is about an enhancement (aka new feature) label Aug 26, 2018
@zonky2 zonky2 added this to the 2.2.0 milestone Aug 26, 2018
@zonky2 zonky2 modified the milestones: 2.2.0, 2.0.4 Aug 26, 2018
@richardhj
Copy link
Member Author

richardhj commented Aug 26, 2018

@baumannsven has had the same idea.

@zonky2
Copy link
Contributor

zonky2 commented Aug 26, 2018

na dann... ;-)

@baumannsven
Copy link
Member

@richardhj Please add author

@baumannsven
Copy link
Member

@richardhj If this pull request merged can I remove the step 2 by the create handler from DCG. Can you test it, and give me information about this?

@discordier
Copy link
Member

The problem I see with this is, now it is implicitly mandatory for all attributes to accept null as valid value where as it was simply "not there" before.

I wonder if this might cause problems.

For your addition of getEmptyValue() to the attributes, I'd rather not bloat the interfaces more but introduce a new interface for these cases. We also want to split up the monolithic attribute classes and interfaces on our route to 3.0, where adding new features to the current (and then deprecated) interfaces would do more harm than benefit.

@richardhj
Copy link
Member Author

The problem I see with this is, now it is implicitly mandatory for all attributes to accept null as valid value where as it was simply "not there" before.

Wow, didn't know there is something more null than null :-D

But I get your point. Because they now get triggered to process null whereas they might not have been called.

I wonder if this might cause problems.

Just now, e.g. the valueToWidget() method of the select attribute does not check for empty value. This is what you ment, right?

For your addition of getEmptyValue() to the attributes, I'd rather not bloat the interfaces more but introduce a new interface for these cases. We also want to split up the monolithic attribute classes and interfaces on our route to 3.0, where adding new features to the current (and then deprecated) interfaces would do more harm than benefit.

Yes, we should discuss the desired interfaces somewhen.

@richardhj
Copy link
Member Author

On the other hand, using $item->set('name', null); have been a valid way to unset a value.

@zonky2 zonky2 added the Up for discussion This ticket will be up for discussion in one of our next calls label Aug 27, 2018
@richardhj
Copy link
Member Author

@baumannsven
The Two-step-save can be reverted. See contao-community-alliance/dc-general#425.

I reverted the two-step save and applied these changes. The alias/combinedvalues are working.

Fixes cases in which you create an item and the alias or combined values attribute does not get notified due `isAttributeSet()` returning false.
@richardhj richardhj force-pushed the hotfix/initialize-attributes branch from be1d3ad to bb20586 Compare August 28, 2018 15:41
@zonky2 zonky2 removed the Up for discussion This ticket will be up for discussion in one of our next calls label Aug 28, 2018
@baumannsven baumannsven self-requested a review August 28, 2018 19:18
@zonky2 zonky2 changed the title Initialize attributes with empty values. [RTM] Initialize attributes with empty values. Aug 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement This issue is about an enhancement (aka new feature)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants