Guide for sending pull requests

172 views
Skip to first unread message

Ingo Schommer

unread,
Oct 8, 2012, 1:17:33 PM10/8/12
to silverst...@googlegroups.com
Hello fellow githubbers!

I've created/adapted a guide detailing our git workflow,
and the expectations we have around pull requests:

Roland Lehmann pointed out to me that the guide wasn't really visible
(a tend to link to it a lot in pull requests, but other than that its only
"buried" in the general contribution guidelines at http://doc.silverstripe.org/framework/en/trunk/misc/contributing).

So here you go: If you're unsure what "rebasing" means,
how and when to branch, how to rewrite a pull request, etc,
we now have a (slightly) SilverStripe specific guide!

All the best
Ingo

P.S.: The guide was adapted from the "ThinkUp" project, with permission from Gina Trapani. Thanks very much!

Sam Minnée

unread,
Oct 8, 2012, 8:22:31 PM10/8/12
to silverst...@googlegroups.com
I found the documentation a bit too hidden, so I've refactored the documentation here, and added a reference from the GitHub README.md file.

https://github.com/silverstripe/sapphire/pull/859

xeraa

unread,
Oct 9, 2012, 12:20:22 PM10/9/12
to silverst...@googlegroups.com
What about a Contributing.md - in case you haven't seen https://github.com/blog/1184-contributing-guidelines yet.

Cheers,
Philipp

Anselm Christophersen

unread,
Oct 10, 2012, 3:12:27 PM10/10/12
to silverst...@googlegroups.com
Ingo,
this link doesn't work for me:

I'm getting the "We're sorry... the page doesnt exist..."

Am I missing something?

Anselm

--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To view this discussion on the web visit https://groups.google.com/d/msg/silverstripe-dev/-/Vv7Zw1oakSIJ.
To post to this group, send email to silverst...@googlegroups.com.
To unsubscribe from this group, send email to silverstripe-d...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/silverstripe-dev?hl=en.

Simon J Welsh

unread,
Oct 10, 2012, 3:21:38 PM10/10/12
to silverst...@googlegroups.com
That would be because this part of the docs got cleaned up just after this was posted. http://doc.silverstripe.org/framework/en/trunk/misc/contributing/ details how to contribute, with http://doc.silverstripe.org/framework/en/trunk/misc/contributing/code being specifically about how to contribute code.
---
Simon Welsh
Admin of http://simon.geek.nz/

Ingo Schommer

unread,
Oct 11, 2012, 11:13:29 AM10/11/12
to silverst...@googlegroups.com
Hello Anselm, Simon, That's fixed now, added a redirect.

Ingo Schommer

unread,
Oct 11, 2012, 11:21:14 AM10/11/12
to silverst...@googlegroups.com
Didn't know about that, github continues to surprise :)
I've added that file to installer/framework/cms projects,
and copied the content from the landing page.
Its a little duplication, but mostly link pointers anyway.

--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To view this discussion on the web visit https://groups.google.com/d/msg/silverstripe-dev/-/VkAeK3oTaBoJ.
Message has been deleted
Message has been deleted

Sam Minnée

unread,
Oct 11, 2012, 5:02:27 PM10/11/12
to silverst...@googlegroups.com
I have a patch that only sets the session cookie when the session is actually used. Although my use-case was making SilverStripe perform better through caching proxies, it should help you here. I'll dig it up.

On 12/10/2012, at 10:00 AM, Michael van Schaik wrote:

> Right now, preventing a cookie from being set, requires some hacking of
> the core, since at least a session is always started (hardcoded) from
> the session class. Would the approach taken in the Cookie class
> enhancement [1] by Matt Lewis also be the way to go for the Session class?
>
> As I understand, thanks to this enhancement it is now possible to
> subclass the Cookie class and prevent a cookie from being set until
> permission has been given. I need this functionality for a session
> cookie as well, so I'd then try to implement the same get_inst()
> functionality on that class. But there's been some discussion if instead
> the Injector class shouldn't be used for this kind of enhancements. As
> far as I can tell though, that would require modifications of any code
> now using the session class throughout the core, right?
>
> (The new cookie law in the Netherlands doesn't allow a website to set
> any cookies (session/non-session) without first getting approval. This
> is a bit stricter than UK, where I think you just have to inform that a
> cookie has been set.)

Sam Minnée

unread,
Oct 11, 2012, 5:27:10 PM10/11/12
to silverst...@googlegroups.com

> Right now, preventing a cookie from being set, requires some hacking of
> the core, since at least a session is always started (hardcoded) from
> the session class. Would the approach taken in the Cookie class
> enhancement [1] by Matt Lewis also be the way to go for the Session class?

OK, I stand corrected, the patch I thought had been missed *is* present in SS3. The session cookie is only created if the cookie is actually used.

The approach would need to be different in the session because the session are currently bound to the controller, and constructed in Director::direct(). Dependency injection is probably your best bet here, so that the Injector is used to construct the session object in both Director::direct() and Session::current_session().

You will also need to refactor Session::start() so that Director::direct() calls a new method, inst_start() on the instance it creates, and that Session::start() calls inst_start(). In fact, the code that checks the $_COOKIE and $_REQUEST in Director can be moved to be within the default Session implementation.

> As I understand, thanks to this enhancement it is now possible to
> subclass the Cookie class and prevent a cookie from being set until
> permission has been given.

Given the session cookie is only created when you need it, your site is going to break if you just disable cookies. That alternative solution is to disable all CSRF or remove all forms from your site until the permission has been given.

> I need this functionality for a session
> cookie as well, so I'd then try to implement the same get_inst()
> functionality on that class. But there's been some discussion if instead
> the Injector class shouldn't be used for this kind of enhancements. As
> far as I can tell though, that would require modifications of any code
> now using the session class throughout the core, right?

The injector is only needed when you *create* the session object for non-test situations - Director::direct() and Session::current_session(). The session doesn't actually do anything unless Session::start() or Session::save() is called.

> (The new cookie law in the Netherlands doesn't allow a website to set
> any cookies (session/non-session) without first getting approval. This
> is a bit stricter than UK, where I think you just have to inform that a
> cookie has been set.)

Blocking of session cookies as well as persistent ones is retarded. Hopefully this law is reverted / amended soon, because it makes no sense whatsoever.

matt clegg

unread,
Oct 11, 2012, 5:28:07 PM10/11/12
to silverst...@googlegroups.com
@Sam, Can that patch be added to the core so you can easily turn it on/off from mysite/_config.php?

@Michael: Actually in the UK you can simply create a link to a cookie policy through 'implied consent' that states something along the lines of; cookies will only be used if necessary, will improve website performance (analytic etc), yadie yada yada...

An example cookie statement is conveniently setup at http://cookiestatement.eu or (for convienience) http://cookiestatement.eu/getthecode

Also, If someone from the community would like to write about how cookies are used in an average Silverstripe session please contact me to help setup; http://silverstripe.cookiestatement.eu/ or if another SS community developer could provide another loophole to get round this stupid law?

Matt




--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To post to this group, send email to silverst...@googlegroups.com.
To unsubscribe from this group, send email to silverstripe-d...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/silverstripe-dev?hl=en.




--


Matt Clegg


Sam Minnée

unread,
Oct 11, 2012, 5:44:09 PM10/11/12
to silverst...@googlegroups.com
@Sam, Can that patch be added to the core so you can easily turn it on/off from mysite/_config.php?

The patch is actually already in core.  The session is created on-demand, which means that if you don't have any CRSF-enabled forms on a page, or other things that use the session, you won't get a session cookie created.  I don't see any need in being more aggressive in creating a session cookie unnecessarily, so it's not a config option -- It's Just Better Now(tm)

Also, If someone from the community would like to write about how cookies are used in an average Silverstripe session please contact me to help setup; http://silverstripe.cookiestatement.eu/ or if another SS community developer could provide another loophole to get round this stupid law?

The main ones, from most common for visitors to least common:

Session
 - CSRF-protection of forms
 - Form error messages
 - Tracking log-in
 - GridField state
 - A bunch of state stuff in the CMS after you're logged in

Cookie:
 - A PastMember cookie is set that recognises someone who visits who previously logged in
 - If you are viewing the draft site (you need to be a CMS author to do this) a bypassStaticCache cookie is set to ensure that you don't see static-cache content
 - If you check "remember me" a cookie "alc_enc" is set

Michael van Schaik

unread,
Oct 12, 2012, 3:17:19 AM10/12/12
to silverst...@googlegroups.com
@Sam, thanks for the clarification on the session cookie, since it's
only created when needed for the CSFR & other form stuff, it can be
classified as a 'technical cookie', necessary for the site to work. This
is already a circumstance under which it's allowed to place the cookie
without warning (at least in NL).

I'd just have to describe these technical reasons in a cookie statement
listing all cookies & what they're used for. And block non-session
cookies until approved (which is already possible). The approval for
logged in users can be added in one line "Allow this site to set
cookies" on the login form, a permission we'll then have to write to a
database or log file along with IP, and keep for 5 years(!).

> Blocking of session cookies as well as persistent ones is retarded.
>Hopefully this law is reverted / amended soon, because it makes no
sense whatsoever.

Absolutely. It's based on an EU directive focussed on online privacy.
The Dutch law implementing this was probably written using a goose
feather on parchment by someone thinking cookies are little applications
on their own, eating away your harddisk (and privacy). No developer,
online entrepreneur nor modestly knowledgeable politician thinks this
law is anywhere near sensible/implementable in it's current form. But
the OPTA (Dutch authority) in all its wisdom has stated it will be
starting enforcement from jan 1st 2013, with high fines.

Up to us to deal with it...
>> <mailto:silverst...@googlegroups.com>.
>> To unsubscribe from this group, send email to
>> silverstripe-d...@googlegroups.com
>> <mailto:silverstripe-dev%2Bunsu...@googlegroups.com>.
>> For more options, visit this group at
>> http://groups.google.com/group/silverstripe-dev?hl=en.
>>
>>
>>
>>
>> --
>>
>>
>> Matt Clegg
>>
>> --https://secure.38degrees.org.uk/tax-dodging-48-hours-left
>>
>>
>> --
>> You received this message because you are subscribed to the Google
>> Groups "SilverStripe Core Development" group.
>> To post to this group, send email to silverst...@googlegroups.com
>> <mailto:silverst...@googlegroups.com>.
>> To unsubscribe from this group, send email to
>> silverstripe-d...@googlegroups.com
>> <mailto:silverstripe-d...@googlegroups.com>.

Sam Minnée

unread,
Oct 12, 2012, 3:23:41 AM10/12/12
to silverst...@googlegroups.com
It'd be good to get something written up on doc.silverstripe.org

Nicolaas Thiemen Francken - Sunny Side Up

unread,
Oct 12, 2012, 4:01:05 AM10/12/12
to silverst...@googlegroups.com
As you say Michael, the key thing is that technically required cookies can be placed without permission (http://www.opta.nl/nl/actueel/alle-publicaties/publicatie/?id=3647).  Are there any instances within the SS core where it places a technically not-required cookie (be it in 2.4 or 3.0?).  

"5 Juni 2012 is de nieuwe cookiebepaling van kracht geworden. Kort gezegd moeten websites bezoekers informeren en toestemming vragen voordat zij een cookie of andere gegevens op bijvoorbeeld een computer, laptop of mobiele telefoon plaatsen of lezen. Dit hoeft niet voor cookies die strikt noodzakelijk zijn voor de werking van de website, aldus de wet. OPTA handhaaft deze bepaling en kan zo nodig boetes opleggen van maximaal 450.000 euro"



Stevie Mayhew

unread,
Oct 12, 2012, 4:08:04 AM10/12/12
to silverst...@googlegroups.com
I'm fairly sure that the only session/cookie which is created is the Session cookie, which as said previously is technical and also expires when the session ends (advertising go):


This leaves us in a fairly good place for having to alert users. Saying that, I've recently been working on a couple of commerce sites with international reach and it really does leave us in a very confused state until the law is proven in court and we learn what is "bad".

Stevie

--

Stevie Mayhew Developer
Ph. 04 831 5130   heyday.co.nz

Heyday is a digital agency based in Wellington, New Zealand. It employs 25 staff and drives the online presence of brands through insight, ideas, design, delivery and improvement. Clients include Weta, Meridian Energy, GIB, Telecom, Pink Batts, Gallagher Group and Z Energy. Please visit our website for further information.




On 12/10/2012, at 9:01 PM, Nicolaas Thiemen Francken - Sunny Side Up wrote:

As you say Michael, the key thing is that technically required cookies can be placed without permission (http://www.opta.nl/nl/actueel/alle-publicaties/publicatie/?id=3647).  Are there any instances within the SS core where it places a technically not-required cookie (be it in 2.4 or 3.0?).  

"5 Juni 2012 is de nieuwe cookiebepaling van kracht geworden. Kort gezegd moeten websites bezoekers informeren en toestemming vragen voordat zij een cookie of andere gegevens op bijvoorbeeld een computer, laptop of mobiele telefoon plaatsen of lezen. Dit hoeft niet voor cookies die strikt noodzakelijk zijn voor de werking van de website, aldus de wet. OPTA handhaaft deze bepaling en kan zo nodig boetes opleggen van maximaal 450.000 euro"




Sophie Dennis

unread,
Oct 13, 2012, 3:30:21 PM10/13/12
to silverst...@googlegroups.com
I recently went through this for a UK site we built using SilverStripe (v2). I decided that the session was exempt from permission as "technically necessary" but that the PastMember cookie was not. My understanding is that we still need to provide information on how the "technically necessary" stuff is used. As a UK site we are have, however, taken the implied consent route for now.

Here is the wording we settled on regarding SS:

== Content Management System ==
The content management system we use to publish our website uses cookies to store some basic, essential data on your interactions with it, such as whether you have logged in. Other cookies tell us if you have previously logged into the website, and are optionally used to remember your login details for future visits if you ask us to.

* PHPSESSID: this is used during your visit to remember any preferences and settings you might ask us to set, such as whether or not you are logged in. If you block this cookie parts of the site will not work properly or will be inaccessible to you. Deleted when you leave the site.

* PastMember: used to remember if and when you have previously logged in. This cookie does not store personal information about you, but if you are a registered user it is used to update your personal record in our database. Stored for 90 days.

* alc_enc: only set if you tick the "Remember email and password for next time" box when you login. Stored for 90 days.

- ends -

Assuming the above is accurate I'm happy for this to form the basis for some standard wording in docs.

See http://www.ouem.co.uk/cookies/ for the full thing which also covers the site's use of GA.

OTHER SOURCES
In preparing the above I found these to be very useful:

Action for Blind People cookie policy (their site uses SS)
http://www.actionforblindpeople.org.uk/member-pages/register/terms-and-conditions/privacy-and-cookies/

Cookie Policy Template
http://www.keymultimedia.co.uk/2012/05/22/eu-cookie-law-template-privacy-wording/

Sophie
---------------

Sophie Dennis, Cayenne
Content Strategy & UX Design | DEVON & OXFORD www.cayenne.co.uk

t: 0845 486 0108
tw: @sophiedennis
skype: cayenneweb
blog: www.sophiedennis.co.uk

Legal bits: Cayenne Web Development Limited is registered in England & Wales no 4502369, at Bloxham Mill, Barford Road, Bloxham, Oxfordshire OX15 4FF.

matt clegg

unread,
Oct 13, 2012, 8:04:25 PM10/13/12
to silverst...@googlegroups.com
Thanks Sam & Sophie,

I have used your copy to update: http://silverstripe.cookiestatement.eu/

Hopefully it would be useful for someone to have a generic cookie statement to reference.

Matt




--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To view this discussion on the web visit https://groups.google.com/d/msg/silverstripe-dev/-/Z-f5hu4uPM0J.

micschk

unread,
Oct 15, 2012, 9:26:37 AM10/15/12
to silverst...@googlegroups.com
The approach would need to be different in the session because the session are currently bound to the controller, and constructed in Director::direct().  Dependency injection is probably your best bet here, so that the Injector is used to construct the session object in both Director::direct() and Session::current_session(). 
You will also need to refactor Session::start() so that Director::direct() calls a new method, inst_start() on the instance it creates, and that Session::start() calls inst_start().  In fact, the code that checks the $_COOKIE and $_REQUEST in Director can be moved to be within the default Session implementation. 

@sam: I've tried to get my head around the Injector class & how this should be used... Read the Silverstripe docs and some texts over at Symfony2. Just to make sure I'm implementing it the right way, thought I'd post back with some code;

Director::direct();

// Only resume a session if its not started already, and a session identifier exists (check moved to session::start())
Session::start();
// Initiate an empty session - doesn't initialize an actual PHP session until saved (see belwo)
//$session = new Session(isset($_SESSION) ? $_SESSION : null);
$session = Injector::inst()->createWithArgs('Session', (isset($_SESSION) ? array($_SESSION) : array()) );

Session::current_session();
...
self::$default_session = Injector::inst()
->createWithArgs('Session', (isset($_SESSION) ? array($_SESSION) : array()) );
 
Session::start();
public static function start($sid = null) {
// Only resume a session if its not started already, and a session identifier exists
if(isset($_SESSION) && (!isset($_COOKIE[session_name()]) || !isset($_REQUEST[session_name()]))) return;
$path = self::get_cookie_path();
$domain = self::get_cookie_domain();
$secure = self::get_cookie_secure();
$session_path = self::get_session_store_path();

if(!session_id() && !headers_sent()) {
if($domain) {
session_set_cookie_params(self::$timeout, $path, $domain, $secure /* secure */, true /* httponly */);
} else {
session_set_cookie_params(self::$timeout, $path, null, $secure /* secure */, true /* httponly */);
}

// Allow storing the session in a non standard location
if($session_path) session_save_path($session_path);

// @ is to supress win32 warnings/notices when session wasn't cleaned up properly
// There's nothing we can do about this, because it's an operating system function!
if($sid) session_id($sid);
@session_start();
}
}

Would this be the right place to ask about this code, or should I post it to the forum?
I have to admid that I don't really understand what the Session::start() is doing, along with the test for resuming a session... 
Also, I'm not sure how I could now use another classname for the Session?

micschk

unread,
Oct 16, 2012, 3:48:36 AM10/16/12
to silverst...@googlegroups.com
Anyone? I just need a heads up if this code is the right approach or not... I'll move it over to the forum as well...

Sophie Dennis

unread,
Oct 16, 2012, 1:49:45 PM10/16/12
to silverst...@googlegroups.com
Hi Mic,

I can't comment on the code as it's beyond my expertise, but if this is *just* for the php session, then I think the consensus was that this can be classed as "strictly necessary" for the site to function. So you do not need consent to set it. So there is no need to refactor the session-related code.

All you need to do is explain its use in a cookie/privacy policy or similar.

Have I got that right?

Sophie

micschk

unread,
Oct 17, 2012, 2:39:54 AM10/17/12
to silverst...@googlegroups.com
Hi Sophie,

Thanks for your comments, your policy text is very useful! I agree about the strict necessity of the session, but decided I'd try and take a stab at making the Session class more extendable by having it initialized via the new SS3 Injector class as per Sam's bullet points anyway. As said though, I'm not sure if I'm implementing it the right way. Not getting much feedback, so I guess I'm either doing something foolishly wrong, or not asking for it in the right place(?)

I'll try my luck over at the forums. Would be nice to hear anything from a dev though, even if the code is totally off (just trying to help).
Reply all
Reply to author
Forward
0 new messages