literal field added multiple times when added to a data-extension

[sunnysideup]

I added a data extension to member. In the updateCMSFields I added a Literal Field. In the CMS this same field shows twice. When I add a private static variable to updateCMSFields to check if the method has already run once (start as false, set to true once running updateCMSFields, do not run again if set to true), the literalfield only gets added once. however, this has other unforeseen consequences.

My theory is that there is no "unique" name for a literal field so it can be added multiple times under the same name.

A second improvement could be to prevent the updateCMSFields from running twice thereby speeding up the CMS?

[chillu]

I've added the following on 3.1 and it only adds once:

<?php
class MemberExtension extends DataExtension {
    public function updateCMSFields(FieldList $fields) {
        $fields->addFieldToTab('Root.Main',
            new LiteralField('Bla', 'blubb')
        );
    }
}

The LiteralField in fact has a unique name.

[sunnysideup]

Hi Ingo

I have added the following code to 3.1.2 and it adds twice (i.e. I see the link appear on the screen two times!):

<?php

class MemberExtension extends DataExtension {

    public function updateCMSFields(FieldList $fields) {
        $fields->addFieldToTab(
            "Root.MyTest",
            new LiteralField(
                "Test1",
                "<p><a href=\"test\">link 1</a></p>"
            )
        );
    }

}

I add the MemberExtension like this:

---
Name: mysiteconfig
After: 'framework/*','cms/*'
---

Member:
  extensions:
    - MemberExtension

Here is the screenshot:

[sunnysideup]

Note that there are two issues here:

1. why does the link show twice (same name should not duplicate even if the code runs 100 times)?
2. why does the code run two times (inefficient I guess)?

[sunnysideup]

ohhh and thank you for looking at this so promptly.

[sunnysideup]

When I add the field to Root.Main it only shows once, so the key factor for solving (1) NOT (2) is that the tab makes a difference.

[sunnysideup]

Just to be sure, I also tried to add a Text Field as above (rather than a literal field). This only adds once - not twice.

[kinglozzer]

I can replicate this behaviour on the 3.1 branch with the above comment (https://github.com/silverstripe/silverstripe-framework/issues/2827#issuecomment-35040896). Seems updateCMSFields() is being called twice - once in Member->getCMSFields() and once in DataObject->getCMSFields(), which is expected, right?

It is actually being added twice to ‘Root.Main’ as well, though it’s only showing once (no idea why, didn’t care to investigate as that’s not the issue here).

Possible fix is to record fields that we’ve already output, something like: https://github.com/kinglozzer/sapphire/commit/f3354bc62364751922e891418ece601183c0e8ad. This could potentially cause fields to disappear from existing sites if anyone has two fields with the same name, though I’m not sure if/why anyone would.

Thoughts?

[sunnysideup]

Why not ensure getcmsfields only runs once ???( On Feb 15, 2014 12:44 PM, "Loz Calver" notifications@github.com wrote:

I can replicate this behaviour on the 3.1 branch with the above comment (#2827 (comment)https://github.com/silverstripe/silverstripe-framework/issues/2827#issuecomment-35040896). Seems updateCMSFields() is being called twice - once in Member->getCMSFields() and once in DataObject->getCMSFields(), which is expected, right?

It is actually being added twice to ‘Root.Main’ as well, though it’s only showing once (no idea why, didn’t care to investigate as that’s not the issue here).

Possible fix is to record fields that we’ve already output, something like: kinglozzer@f3354bchttps://github.com/kinglozzer/sapphire/commit/f3354bc62364751922e891418ece601183c0e8ad. This could potentially cause fields to disappear from existing sites if anyone has two fields with the same name, though I’m not sure if/why anyone would.

Thoughts?

— Reply to this email directly or view it on GitHubhttps://github.com/silverstripe/silverstripe-framework/issues/2827#issuecomment-35139637 .

[kinglozzer]

@sunnysideup Member->getCMSFields() calls parent::getCMSFields() to scaffold most of the fields, both getCMSFields() methods have the ->extend() hook called so updateCMSFields() is called twice.

We could rewrite Member->getCMSFields() to not call parent::getCMSFields(), but it’s just hiding the problem that will likely resurface somewhere else.

[tractorcow]

Another possible (and perhaps the 'correct') solution is to make use of beforeUpdateCMSFields in Member::getCMSFields. (https://github.com/silverstripe/silverstripe-framework/blob/3.1/docs/en/reference/dataextension.md#addingmodifying-fields-prior-to-extensions)

This method is already a behemoth though; If clarity and readibility is an issue, I suggest to go with @kinglozzer idea to rewrite it without the parent call. I wouldn't say that it hides the issue; It would only resurface in cases where Member was extended, and even the only if the subclass doesn't utilise beforeUpdateCMSFields properly, which is going to be a problem regardless.

What it would do is certainly solve the issue of the result of automatic scaffolding needing "fixing" anyway, which is a problem in itself. A quarter of the method is spent just replacing or removing fields, which demonstrates that the parent call is inappropriate in this case anyway.

This solution would resulting in a change of behaviour for 3.1, since extensions which added data fields would subsequently need to implement formfield alterations in updateCMSFields (which is how the game should have been played from the start) so I suggest a target of 3.1.3 for this, if not 3.2.

A possible third alternative is the extension disabling 'hack' that the blog module uses, but I prefer to stick to conventions over hacks, since that method can't be nested in subclasses.

[sunnysideup]

Member should not extend ... Or have a different extend. On Feb 16, 2014 10:34 PM, "Loz Calver" notifications@github.com wrote:

@sunnysideup https://github.com/sunnysideup Member->getCMSFields()calls parent::getCMSFields() to scaffold most of the fields, both getCMSFields()methods have the ->extend() hook called so updateCMSFields() is called twice.

We could rewrite Member->getCMSFields() to not call parent::getCMSFields(), but it’s just hiding the problem that will likely resurface somewhere else.

— Reply to this email directly or view it on GitHubhttps://github.com/silverstripe/silverstripe-framework/issues/2827#issuecomment-35180886 .

[kinglozzer]

@tractorcow I wasn’t even aware of the existence of beforeUpdateCMSFields(), that definitely looks like the best solution to me if we can make it work.

Just to clarify the usage of it, for this example we’d move the addition of any new fields into beforeUpdateCMSFields(), then call parent::getCMSFields() before modifying/removing existing fields? And remove the second ->extend() hook, right?

[tractorcow]

Na, you can do it entirely in the call back, since it triggers AFTER field scaffolding, but before extensions. On Feb 17, 2014 12:42 AM, "Loz Calver" notifications@github.com wrote:

@tractorcow https://github.com/tractorcow I wasn't even aware of the existence of beforeUpdateCMSFields(), that definitely looks like the best solution to me if we can make it work.

Just to clarify the usage of it, for this example we'd move the addition of any new fields into beforeUpdateCMSFields(), then call parent::getCMSFields() before modifying/removing existing fields? And remove the second ->extend() hook, right?

Reply to this email directly or view it on GitHubhttps://github.com/silverstripe/silverstripe-framework/issues/2827#issuecomment-35194682 .

[sunnysideup]

I would like to propose an alternative fix ...

SEE: https://github.com/silverstripe/silverstripe-framework/pull/2851

note: UserDefinedForm suffers from the same problem @wilr

[tractorcow]

@sunnysideup Is it necessary to add additional conventions / API? Now users who want to extend Member need to (potentially) implement two methods rather than one.

[sunnysideup]

@tractorcow , yes, I think this is the best solution... Because it is not a hack. People can still use updateCMSFields, as per usual, but they can do some fine-tuning - if needed, using updateMemberCMSFields. This same pattern can be applied to any other DataObject that adds a hook (e.g. updateUserDefinedFormCMSFields) while the updateCMSFields is always available through the DataObject::getCMSFields method.

[sunnysideup]

I am not so enamoured with https://github.com/silverstripe/silverstripe-framework/pull/2850/files, because it seems to me more like a workaround rather than a real fix. It is very smart and elegant, but in general we always call parent::getCMSFields first - and so it is very hard to read this new code as it goes against all the conventions... I think adding more hooks with different names makes it a lot easier to extend member and show exactly the fields you want.

[kinglozzer]

@sunnysideup The problem is that we’d have to make API changes (and quite a few of them) in order to fix a fairly small bug. We’d end up rewriting all our extension hooks for fields, (SiteTree, SiteConfig, Member, File/Folder, Group - we’d likely have to do all of these and more) to match this new convention, making upgrades much harder. Plus if we make API changes, this fix will have to be delayed until 3.2.

I don’t think using beforeUpdateCMSFields() is a workaround or hack, this use case is pretty-much what that method was designed for (I think). While I agree it’s slightly harder to understand at first, it makes it much easier to apply extensions - that’s what we want to make as easy as possible - and will require no code changes from developers/module developers.

[sunnysideup]

yo yo.. thank you for the feedback... awesome.

"The problem is that we’d have to make API changes (and quite a few of them) in order to fix a fairly small bug. We’d end up rewriting all our extension hooks for fields, (SiteTree, SiteConfig, Member, File/Folder, Group - we’d likely have to do all of these and more)" -

I am not sure why we have to make any of these changes. I checked CMS + FRAMEWORK for updateCMSFields... Only Member has this crooked construction right now. File, folder, group, etc... do not call parent::getCMSField, so they can happily call updateCMSFields (and they should!)

Plus if we make API changes, this fix will have to be delayed until 3.2. - that is fine... I dont think it is a biggie... Just a small speed increase and little clean up.

The reason I dont like using beforeUpdateCMSFields is that adding the fields to the Member CMS form is not really "before"... What we may consider instead is to remove the parent::getCMSFields, just like they do in File, Folder, Group, and SiteTree. However, the general pattern of having updateCMSFields being called from DataObject and adding the hook "updateMyDataObjectCMSFields" to your custom DataObjects is a solid pattern I reckon... the updateCMSFields is called fairly early in the piece and the updateMyDataObjectCMSFields is right at the end of the collation of CMS Fields.

[tractorcow]

I agree with you @sunnysideup on removing the parent call, but I still don't like the idea of adding extra extension points.

The reason the rest of the framework doesn't have this issue is because Member is relying on the auto-scaffolding to setup most of the fields, where classes such as SiteTree simply generate the FieldList explicitly, and then call the extensions afterwards. If we were to be consistent, we should probably write this method in the same fashion, and then leave the extension hooks up to subclasses / extensions in user code (and other third party modules). This is still my preferred approach, and doesn't require any additional API or extra hooks, so those problems would be moot. :)

beforeUpdateCMSFields is really a mechanism best left for subclasses of core datobjects, since there are cleaner alternatives in those cases. I don't think @kinglozzer 's code has any major problems, but on second thoughts it's probably not consistent with the rest of the framework. Sorry for backtracking on you mate!

[kinglozzer]

@sunnysideup Sorry if you thought I was being a bit short with you or anything, you’re right in that there wouldn’t be many methods that need their own ‘new’ hook - I should’ve double-checked that before commenting!

@tractorcow No worries, in the interests of consistency it does make more sense. I should’ve confirmed that was the way we were going before writing a patch anyway!

@sunnysideup If you’re happy with this as a solution, I’ll leave the rewrite of getCMSFields to you if that’s okay? Let me know if not, just want to avoid us both writing the same patch :)

[sunnysideup]

@kinglozzer - not short at all... awesome to see all the great work

So - we keep Member as it is EXCEPT that we remove the parent::getCMSFields call? and fix whatever need fixing after we remove it?

[tractorcow]

@kinglozzer @sunnysideup Well, it would involve replacing the parent::getCMSField with all the default field generation not handled in the rest of the function, but yes essentially that's what we're looking at. :)

[sunnysideup]

Hi @kinglozzer and @tractorcow

Please have a look at the proposed pull request: https://github.com/silverstripe/silverstripe-framework/pull/2857.

Please tear it down ;-) First go at this...

Cheers

Nicolaas

[sunnysideup]

what we have not worked out yet is why the literal field showed twice .... ;-)

[kinglozzer]

Hey @sunnysideup. The literal field showed twice because updateCMSFields() was called twice - we’ll prevent this by removing the call to parent::getCMSFields(), so updateCMSFields() will only be triggered once.

Something went wrong with your pull request, which is why Simon closed it. As this is a bugfix, not a new/changed API, the pull request should be opened against the 3.1 branch instead of master (do this by running git checkout 3.1 before git branch your-branch-name).

The contributing docs: http://doc.silverstripe.org/framework/en/trunk/misc/contributing/code and in particular this image: http://www.silverstripe.org/assets/doc-silverstripe-org/collaboration-on-github.png helped me a lot when I first started using git & Github :)

[sunnysideup]

ahh.... I never know if it is MASTER / 3.1 BRANCH... ok - I will have another go at it... THANK YOU @kinglozzer

[sunnysideup]

Here is the new pull request ... https://github.com/silverstripe/silverstripe-framework/pull/2860

[kinglozzer]

That looks good @sunnysideup, I’ll wait for Travis to finish and look through it :)

[ajshort]

This kind of change should actually go against master, since it probably requires anyone using the member CMS fields to change their code. Ideally anything written for 3.1 should run across all 3.1.x versions.

[tractorcow]

@ajshort To add to that, there might need to be a note made in the upgrading docs about extensions to Member necessitating an update to their updateCMSFields method.

Also @sunnysideup do you mind to please add a test case? You can copy @kinglozzer 's tests verbatim if you like. :)

Really good work mate! :+1:

[kinglozzer]

@ajshort Apologies, I thought it would be okay for 3.1 as this issue doesn’t affect data fields, only LiteralField and other non-data field types, so any changes would be minor ‘hack removals’.

Sorry @sunnysideup!

[tractorcow]

@chillu Do you want to weight in on this topic? Couldn't we merge this into 3.1 after 3.1.3 is released?

[sunnysideup]

I am wondering if data-extensions of member with additional fields (e.g. MiddleName) would be added automatically to the CMS Fields using the old system (parent::getCMSFields) and NOT with the new code (build fields from scratch). I will look into this... If they were added automatically with the old system then I think we should keep that feature by running something like:

        $extensionFields = $this->scaffoldFormFields(array(
            // Don't allow has_many/many_many relationship editing before the record is first saved
            'includeRelations' => ($this->ID > 0),
            'tabbed' => true,
            'ajaxSafe' => true,
            'restrictFields' => array_diff($this->db(), self::$db)
        ));

Any other feedback would be great. The feedback so far has been super useful!

[tractorcow]

Na, they wouldn't be, hence the suggestion to update the upgrade docs. :)

[sunnysideup]

updated comment above ;-)

[sunnysideup]

I think it would be good to keep the extension fields by default... as this makes live easier for coders.

[tractorcow]

Auto-scaffolded fields are the little MS word paperclip of "It looks like you're trying to create a dataobject. I've added some formfields randomly to help you get started."

[simonwelsh]

I'm not a fan of this approach. It's a rather dramatic change of behaviour, especially since there's a lot simpler methods that keep the current behaviour.

I think that using beforeUpdateCMSFields is still the cleanest option. It does exist for just this reason and there's no reason to not use it beyond classes that were built before it existed, and do create rather custom CMS fields, don't use it.

Otherwise, you're going to need to basically replace the parent::getCMSFields() call with the contents of DataObject::getCMSFields(), or have a rather large overhead to maintain current compatibility using @sunnysideup's suggestion (you'll have to manually merge the tabs and still handle has_ones, and remove the many_manys, but not the ones from extensions).

[sunnysideup]

I am totally lost now ;-)

As far as I can tell, the current suggested fix (https://github.com/silverstripe/silverstripe-framework/pull/2860) is still a solid (albeit unfinished) solution to the small bug I spotted) for the reasons outlined in the comment accompanying the pull request. As an added bonus, it makes the Member CMS Fields easier to use (and less intimidating) for website admins.

For the pull request to go ahead we need the following:

• add to master rather than 3.1 branch
• Upgrade note in 3.2.0.md about updating Member extensions
• automatically add fields from member extensions
• Test case (can copy from https://github.com/silverstripe/silverstripe-framework/pull/2850/files)
• Fix errors: Seems to be a behat error at https://travis-ci.org/silverstripe/silverstripe-framework/jobs/19070227 - Fix Behat test that sets the language to German and checks for the word "Sprache".
• review translatable words. I suggest we change the "locale settings" tab to something less geeky ... e.g. localised formatting.

If I missed anything then please add!

Thank you

Nicolaas

[tractorcow]

Thanks for weighing in on this @simonwelsh ,

In interest of maintaining behaviour for 3.1, could we save https://github.com/silverstripe/silverstripe-framework/pull/2860 for 3.2, and merge https://github.com/silverstripe/silverstripe-framework/pull/2850 into 3.1?

I see @kinglozzer's PR fixing the immediate problem without changing the API, but @sunnysideup's providing the best long term solution that's consistent with the way SiteTree scaffolds fields.

[kinglozzer]

I’ve reopened that PR in case you want to merge it; I’ll step away and leave it to you guys for now, let me know if you need anything!

[tractorcow]

We are just waiting on 3.1.3 to be released, after which we can merge your PR into 3.1, @kinglozzer. I think we agree we can't change the API at this stage.

I'd still nominate @sunnysideup's solution for 3.2, but that can be decided at a later date.

[sunnysideup]

also see #2860

[tractorcow]

Resolved with #2850

[sunnysideup]

I would like to re-open this or open a new ticket, because the current fix is not as good as it can be and not very fast as far as I know. Shall I start new ticket?

[tractorcow]

Hey @sunnysideup. By all means, if you can come up with a more robust long term solution then that would be great. For the mean time, we will accept the current solution as having fixed the problem, but that doesn't mean we can't improve on it.

I personally would have liked #2860, but as @simonwelsh rightly pointed out we can't change the API so much in 3.1, as developers would have to rewrite any workarounds or fixes they implemented against the old method.

I would suggest to rebase your changes against 3.2.