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

Fix incompatibility with Translatable #32

Merged
merged 1 commit into from Mar 14, 2016

Conversation

JorisDebonnet
Copy link

Before, children would always take the default Locale. This fixes that.

@micmania1
Copy link

I wonder if we could add an extension into CMSEditLink so that translatable can handle this itself. http://api.silverstripe.org/3.1/source-class-SiteTree.html#556

The problem with the current solution is that it only fixes the issue for Lumberjack and not any other module which might do similar. It also creates a dependency (although not really because of the if statement) which i'd like to avoid.

I should get time to look at this more on Friday, but its likely that this will get merged as I doubt an ideal solution is possible quite yet.

@tractorcow
Copy link

Or could you set the locale via updateDefaults() on sitetree?

At the moment it's set via "defaults" config fixed to the default locale. Change it to set the current locale in Translatable::populateDefaults() and boom fixed. :D No new extension points needed.

@tractorcow
Copy link

I concur with @micmania1 ; This is a translatable issue not a lumberjack one.

@JorisDebonnet
Copy link
Author

Well I also agree; as long as it gets fixed somehow, I'll be happy ;)

@dhensby dhensby closed this Mar 1, 2016
@JorisDebonnet
Copy link
Author

As this issue has been closed:
Has this in fact been fixed in Translatable then, or is there an issue about it there?

@micmania1
Copy link

Nope, this hasn't been fixed yet. I'm planning on looking at this tomorrow so i'll re-open until this is confirmed as fixed in translatable.

@micmania1 micmania1 reopened this Mar 3, 2016
@dhensby
Copy link

dhensby commented Mar 3, 2016

Sorry - I closed it because this is mis-assigned. Can someone open an issue against the correct module, please?

@micmania1
Copy link

I've fixed this in translatable and cms silverstripe/silverstripe-translatable#230

To avoid bumping minimum cms and translatable version I'd be happy to merge this with only a slight change. Can you change the if so that it matches the removed CMS check here (without isset on $data['Locale']): https://github.com/silverstripe/silverstripe-cms/pull/1411/files#diff-5a31043a41c8be646444a6d319aa6e37L151

Also, can you comment the code referring to this thread and give a little context as to why we need to refer to Translatable.

@@ -119,6 +119,9 @@ public function handleAction(GridField $gridField, $actionName, $arguments, $dat
"ParentID" => $tmpData['currentPageID'],
"PageType" => $tmpData['pageType']
);
if(class_exists('Translatable')) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if(class_exists('Translatable') && singleton('SiteTree')->hasExtension('Translatable')) { 
    // $data 
}

I think that should be okay

@JorisDebonnet
Copy link
Author

This look good?
If we know which version of Translatable makes this fix obsolete, I could add that in the comment.

@JorisDebonnet
Copy link
Author

I figured it doesn't need the class_exists('Translatable') check if we're already checking whether SiteTree has that Extension available. I tried it: I can still create new blog posts via the Add button after I removed the translatable module from my test.

@micmania1
Copy link

Can you update the comment to link to this pull request. Also, the translatable module doesn't currently do it, but it will in the future so this comment won't actually be true anymore.

@JorisDebonnet
Copy link
Author

Yeah, I know, that's why I wanted to comment something about when it will be available in Translatable. By updating the comment to link to this pull request, you mean in the actual php code? Or in the commit message?

@micmania1
Copy link

Either would do. Preferably both so its as clear as possible 👍

JorisDebonnet added a commit to JorisDebonnet/silverstripe-lumberjack that referenced this pull request Mar 11, 2016
@JorisDebonnet
Copy link
Author

Added the comment, if it makes any sense... if it doesn't, tell me exactly what to write :p

micmania1 added a commit that referenced this pull request Mar 14, 2016
Fix incompatibility with Translatable
@micmania1 micmania1 merged commit b0841d5 into silverstripe:1.1 Mar 14, 2016
@micmania1
Copy link

Thanks @JorisDebonnet

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

4 participants