search

arambalakjian / DataObject-as-Page

A SilverStripe module for displaying DataObjects as Pages
53 stars 27 forks source link

refactor(DataObjectAsPage): Make this class more flexible #18

Closed petebacondarwin closed 11 years ago

petebacondarwin commented 11 years ago

Removes Content field Removes call to parent::getCMSFields so we don't get unwanted scaffolded fields Extracts field creation from getCMSFields into separate protected methods that derived classes can use

BREAKING CHANGES: Content field has been removed - derived classes will have to add this back themselves if it is needed. Scaffolded fields are no longer included in getCMSFields - derived classes can simply use an instance of FormScaffolder to achieve the same result.

petebacondarwin commented 11 years ago

By the way, in 3.0 the label associated with the "Title" field is "Page Name".

petebacondarwin commented 11 years ago

OK, so I have reverted the urlSegment positioning back to underneath the Title field (and not inside the MetaData field). Keen to merge? By the way, if you do it locally, rather than clicking the merge button on GitHub, it will be a Fast Forward merge, which makes your tree look neat and tidy and doesn't add in an extra commit. Pete

arambalakjian commented 11 years ago

Happy to merge this now, but how should I merge without screwing up sites that use the Content field? Should I create a branch for the existing code them merge this into master? What would you call that branch?

Cheers,

Aram

petebacondarwin commented 11 years ago

If you are talking about 3.0 branch (i.e. master) how many sites are actually using this right now? If you are going to start to support people downloading this via Composer then you need to start using semantic version numbers http://semver.org/. As this has a breaking change to your public API then you need to change the major version number. You would also add this to the change log if you have one and the README. It is an easy fix for site that are still using Content, since they just add it to their own Model fields. Pete

On 12 November 2012 10:06, Aram Balakjian notifications@github.com wrote:

Happy to merge this now, but how should I merge without screwing up sites that use the Content field? Should I create a branch for the existing code them merge this into master? What would you call that branch?

Cheers,

Aram

— Reply to this email directly or view it on GitHubhttps://github.com/arambalakjian/DataObjects-as-Pages/pull/18#issuecomment-10282112.

arambalakjian commented 11 years ago

Yes, but they would then lose any existing content as it would be looking for it on a different table.

I'm not sure how many people are using it. I only released it last week, but I assume if people were upgrading from 2.4 to 3.0 then they would still have the breaking issue at any point in the future.

How would I define the Version numbers, with tags or branches?

petebacondarwin commented 11 years ago

Generally, I would have a branch for every version that you are maintaining. I.E. Your 1.0.x, 1.3.x, 2.x, etc. Then if you do bug fixes for those branches you commit to that branch and then do a tag (i.e. a snapshot) and name it accordingly 1.0.1, 1.0.2, 1.3.56, 1.3.57, 2.0.1, 2.0.3, etc.

I am not clear how the Silverstripe ORM works. Can it not do that kind of migration, where you move a field down to a child model?

Pete

On 12 November 2012 10:18, Aram Balakjian notifications@github.com wrote:

Yes, but they would then lose any existing content as it would be looking for it on a different table.

I'm not sure how many people are using it. I only released it last week, but I assume if people were upgrading from 2.4 to 3.0 then they would still have the breaking issue at any point in the future.

How would I define the Version numbers, with tags or branches?

— Reply to this email directly or view it on GitHubhttps://github.com/arambalakjian/DataObjects-as-Pages/pull/18#issuecomment-10282460.

petebacondarwin commented 11 years ago

Also, if you upgrade to a breaking change then you should expect to have to do some work to get it running correctly.

How about, as an alternative, you to create a new class that sits above the current DOAP and provides the minimal functionality but not the fields? Then people, like me, could hook into that rather than the big DOAP?

Pete

On 12 November 2012 10:25, Peter Bacon Darwin pete@bacondarwin.com wrote:

Generally, I would have a branch for every version that you are maintaining. I.E. Your 1.0.x, 1.3.x, 2.x, etc. Then if you do bug fixes for those branches you commit to that branch and then do a tag (i.e. a snapshot) and name it accordingly 1.0.1, 1.0.2, 1.3.56, 1.3.57, 2.0.1, 2.0.3, etc.

I am not clear how the Silverstripe ORM works. Can it not do that kind of migration, where you move a field down to a child model?

Pete

On 12 November 2012 10:18, Aram Balakjian notifications@github.comwrote:

Yes, but they would then lose any existing content as it would be looking for it on a different table.

I'm not sure how many people are using it. I only released it last week, but I assume if people were upgrading from 2.4 to 3.0 then they would still have the breaking issue at any point in the future.

How would I define the Version numbers, with tags or branches?

— Reply to this email directly or view it on GitHubhttps://github.com/arambalakjian/DataObjects-as-Pages/pull/18#issuecomment-10282460.

petebacondarwin commented 11 years ago

There are a couple of other issues around the urlSegment in this change - particularly if there is no $this->ID. Will take a look tomorrow.

arambalakjian commented 11 years ago

Ok cool.

I think that would still break, because existing 'Content' would be in the DOAP table not the subclass. Just did a test and it does indeed just lose the content.

petebacondarwin commented 11 years ago

How about writing a migration? http://api.silverstripe.org/trunk/framework/dev/MigrationTask.html

arambalakjian commented 11 years ago

Hmm, yea could do, but not much info on how to do it. Bearing in mind that SilverStripe will no longer be aware of the Content field in the DataObjectasPages table, so it would most likely need to be written in SQL.

My Raw SQL has been spoiled by Sapphire ;) I'll take a look when I have a min though.

Aram

petebacondarwin commented 11 years ago

Actually thinking about this - perhaps my requirement is different from most other people's and in fact a Title and Content on a DataObjectasPage is a reasonable request. So I will revert that bit but I still think that factoring out each of the CMS fields bits is a good idea.

arambalakjian commented 11 years ago

Ok cool I think your right, you can always do a removeByName() to hide the field from the user. I know it's not ideal as you would have a redundent field, but it does keep it consistent with 'Page'.

petebacondarwin commented 11 years ago

What I am actually thinking now :-) is that you could insert a new (bare bones) class above the DOAP, which doesn't have these fields. Then I could hook into that instead and it wouldn't break people's code already. What do you think of that? Pete

On 15 November 2012 11:44, Aram Balakjian notifications@github.com wrote:

Ok cool I think your right, you can always do a removeByName() to hide the field from the user. I know it's not ideal as you would have a redundent field, but it does keep it consistent with 'Page'.

— Reply to this email directly or view it on GitHubhttps://github.com/arambalakjian/DataObjects-as-Pages/pull/18#issuecomment-10405919.

arambalakjian commented 11 years ago

Hmm, it would still break people sites because it would have a new table that they didn't have as it would need URL segment on it along with all the versioning bits....

petebacondarwin commented 11 years ago

Hmm. OK.

On 15 November 2012 11:56, Aram Balakjian notifications@github.com wrote:

Hmm, it would still break people sites because it would have a new table that they didn't have as it would need URL segment on it along with all the versioning bits....

— Reply to this email directly or view it on GitHubhttps://github.com/arambalakjian/DataObjects-as-Pages/pull/18#issuecomment-10406192.

petebacondarwin commented 11 years ago

By the way, is it not possible to stick a PageType into ModelAdmin?

On 15 November 2012 11:57, Peter Bacon Darwin pete@bacondarwin.com wrote:

Hmm. OK.

On 15 November 2012 11:56, Aram Balakjian notifications@github.comwrote:

Hmm, it would still break people sites because it would have a new table that they didn't have as it would need URL segment on it along with all the versioning bits....

— Reply to this email directly or view it on GitHubhttps://github.com/arambalakjian/DataObjects-as-Pages/pull/18#issuecomment-10406192.

arambalakjian commented 11 years ago

Possible but messy, you need to assign them a parent and then hide them from the site tree and menus. I did do this for one project but found it was cleaner to do it this way.

Especially in SS3 I don't think there is an easy way to hide items from the site tree...

micschk commented 11 years ago

Hey Aram, I found that if you stick a PageType into ModelAdmin, you lose the versioning/publish buttons & settings section when editing. Have you found a generic way to fix that? (DOAP uses versioning)... If so, I have a tiny module that allows to hide certain pages from the sitetree: https://github.com/micschk/silverstripe-excludechildren

arambalakjian commented 11 years ago

Hi Micschk, that will be to do with the getCMSActions issue with SS3, it doesn't call it in GridField which means it doesn't get called in ModelAdmin. DOAP gets around this by subclassing GridField components, take a look you should be able to use the same technique to get those buttons back for SiteTree items too.