Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

petebacondarwin
Copy link
Contributor

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
Copy link
Contributor Author

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

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 them if they don't want to call parent::getCMSFields

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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//pull/18#issuecomment-10282112.

@arambalakjian
Copy link
Owner

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
Copy link
Contributor Author

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//pull/18#issuecomment-10282460.

@petebacondarwin
Copy link
Contributor Author

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//pull/18#issuecomment-10282460.

@petebacondarwin
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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

@arambalakjian
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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//pull/18#issuecomment-10405919.

@arambalakjian
Copy link
Owner

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
Copy link
Contributor Author

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//pull/18#issuecomment-10406192.

@petebacondarwin
Copy link
Contributor Author

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//pull/18#issuecomment-10406192.

@arambalakjian
Copy link
Owner

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
Copy link

micschk commented Jan 3, 2013

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants