RFC: Namespace of Page

[sminnee]

In SS4 we're introducing namespaces to all classes. However, for the Page class, this presents an issue.

The Page class is supplied by a project developer as part of their application.

class BlogPage extends Page {
}

The blog module needs to know what namespace Page sits in. But what namespace will developer put their project code into? Generally speaking, code should be placed into a globally unique namespace, which means that there will be no single namespace within which Page sits.

Accepted solution

class Page extends AppNamespace\Page (option 20160729-B)

Developers can create a small helper file with this boilerplate (or include it in their _config.php):

use Sminnee\AppNamespace as App;

class Page extends App\Page { private static $hide_pagetype = true; }
class Page_Controller extends App\Page_Controller {}

Note that the expectation is that developers write this code themselves rather than have it auto-generated.

Pro:

• There's no "magic", custom page types extend from a real class
• Class manifest doesn't need to be changed

Con:

• A pair of page classes exist in the class hierarchy; anything that performs logic based on walking the class hierarchy will see both classes. HiddenClass helps here
• HiddenClass's behaviour isn't sufficient to hide a class but not its subclasses; we need $hide_pagetype. Probably $hide_pagetype will replace $hide_ancestor

---

Other comments

In both of these options, it's expected that by SilverStripe 5 at least, we'd no longer have to have module maintainers subclassing Page as a way of providing custom functionality. So, the facilities provided by this RFC won't be long-lived. However, exactly how we do that is beyond the scope of this RFC.

Other alternatives

class_alias() (option 20160729-A)

We can create a class loader that lets us define class aliases, e.g. you could provide this in your _config.php

add_class_alias('Sminnee\\DevProject\\Page', 'Page');
add_class_alias('Sminnee\\DevProject\\Page_Controller', 'Page_Controller');

Pro:

• No extra class in the class hierarchy

Con:

• Will need modification of class manifest to recognise these aliases
• Use a hard-coded SSApp\Page namespace Simpler, but it forces people to bake in assumptions about whether a piece of code is in a project or a library. Since a lot of code starts out as a standalone project and then becomes a library over time, this seems awkward (e.g. a CLI tool that you then want to include as a library into a project as an alternative)
• Alias to a hard-coded namespace, via one of the methods above This could be SSApp or __App. It forces a change in module code without a clear benefit, and it's something we expect to deprecate by SS5. Introducing a new API with an immediate plan to deprecate its use doesn't make a lot of sense; better to work with what we have.
• Perform code generation on any module that subclasses Page to pick the right namespace This would be a triumph of cleverness over practicality.
• Move away from subclasses of Page as way of creating page type in modules ...might be the best long term solution, but more of an SS5 thing. It will be pervasive and distracting and we don't have time for it.
• option 20160729-A, but with a more conciseset_app_namespace('Sminnee\\DevProject') syntax. I prefer the former, more explicit syntax.

[wilr]

Leaning towards alternative 1. installer can define a default app namespace. No performance issues around using that vs a hard coded /SilverStripe/Pagetypes/ namespace?

Also, any need to not provide Page.php as part of the CMS module? With extensions and hooks easy to modify. Would simpify bootstrapping as you wouldn't even need an app / mysite folder potentially.

[unclecheese]

I'm glad that very last option is on the list, because the quasi-dependency of some modules on the mysite module is really idiosyncratic and I'd love to see a movement way from that. Clearly not an option for SS4, though.

I have no numbers to back me up on this yet, but my assumption is that the number of modules that depend on Page are few. Most modules these days have few concerns for the frontend. Most are are augmenting the backend in some way, so again, without any real data on that, my instinct is to be a bit more comfortable breaking that small percentage of modules given the amount of time and/or magic we have to create to accommodate them.

What if instead of registering the app namespace globally, each of these modules required the user inform them of the app namespace, e.g. Blog:set_page_namespace(\UncleCheese\Mysite');.

It may feel redundant if the user has many of these Page dependent modules installed, but keep in mind, it is the module that has the dependency on mysite, so it's therefore appropriate for it to require that level of configuration. The benefit is that for the vast majority of users who are not using Page dependent modules, they don't have to worry about this set_app_namespace() magic, and all users get to namespace anyway they see fit without a cryptic \__App in their Page class.

Another thing to think about is why can't we just extend SiteTree?

[sminnee]

Also, any need to not provide Page.php as part of the CMS module? With extensions and hooks easy to modify. Would simpify bootstrapping as you wouldn't even need an app / mysite folder potentially.

The class you describe is called "SiteTree". Moving away from extending-via-subclassing to extending-via-extensions seems like a bigger change and not something that I particularly want to get distracted by for SS4. It might be worth exploring but to date I haven't seen many sites that have avoided any customisations to their Page.php.

We'd want to look at the ways in which Page.php is used to customise default pages and/or the ways in which modules provide their own page types. I feel like it would quickly get into "do we really need page types at all?" which gives a sense of the scale of the discussion.

[camspiers]

I'd like to see how possible it is to make BlogPage not extend Page, but instead SiteTree, and if it is possible then what needs to be done to achieve it. If it isn't too onerous we could encourage that as a pattern for other modules that need to create something like BlogPage where they would previously extend Page. It might be a matter of BlogPage allowing itself to be configured by the app to use a particular template etc. But I can imagine it would eventually result in needing to extend BlogPage in your particular app, and then use a trait to share common implementation that the Page template needs access to (menu functionality, common code like includes, analytics etc).

[sminnee]

I have no numbers to back me up on this yet, but my assumption is that the number of modules that depend on Page are few.

Anyone want to get some numbers? Off the top of my head blog, userforms, forum, ecommerce, memberregistrations, iframe. It's pretty prevalent.

I'd like to see how possible it is to make BlogPage not extend Page, but instead SiteTree, and if it is possible then what needs to be done to achieve it. If it isn't too onerous we could encourage that as a pattern for other modules that need to create something like BlogPage where they would previously extend Page. It might be a matter of BlogPage allowing itself to be configure by the app to use a particular template etc.

I think it would probably be easier to do away with custom page types as a pattern for module creation, and instead standardise on a content blocks system, and encourage developers to provide a blogblock, userformblock, forumblock, etc.

But "do away with page types" isn't small, and it's beyond the scope of this RFC. We've got enough yak shaves as it is.

[camspiers]

Off the top of my head blog, userforms, forum, ecommerce, memberregistrations, iframe. It's pretty prevalent.

Yeah that is quite a few.

I think it would probably be easier to do away with custom page types as a pattern for module creation, and instead standardise on a content blocks system, and encourage developers to provide a blogblock, userformblock, forumblock, etc.

No comment on the blocks system or doing away with custom page types, but I do agree with the thrust of the RFC that there needs to be a not too invasive upgrade path for modules currently extending Page.

As to alternative 1, this isn't necessarily an issue, but an implication of the solution is that any code that statically refers to BlogPage must be executed after mysite/_config.php is called, otherwise the class \_App\Page would fail to be loaded. Probably an uncommon occurrence which would likely only happen for module that refer to BlogPage in their config (and those that are alphabetically earlier than mysite, if that is the way things still work).

[sminnee]

But an implication of the solution is that any code that statically refers to BlogPage must be executed after mysite/_config.php is called, otherwise the class _App\Page would fail to be loaded

We could potentially implement this as a composer plugin, so that composer's autoloader takes care of the aliasing.

[patricknelson]

Speaking of smooth upgrade paths :smile: Isn't Page a special case, as it's by default installed into the mysite/code/ directory? That being the case, why do anything here? It's in the global namespace, always is and why can't it continue to be?

Put it this way -- most cases it shouldn't cause conflict. If/when it does, SS 4.0 should be capable of being extensible enough to define the fully qualified class name for Page in case it doesn't exist at \Page. e.g. Default to \Page (good for backward compat but also just ease of use) and allow users to define the location of the base Page in their own way.

This helps prevent constraining just to Page as the base class name, if necessary. Heck, even an external out-of-date module could depend on \Page when your main Page instance is actually \MySite\MyPage and, if that's the case, you could also just setup another PHP class in global namespace Page extends MySite\MyPage and fix the module easily. No?

[patricknelson]

p.s. That being said, if we need to configure and/or otherwise tell core where my version of Page exists beyond that, I favor @unclecheese's approach here.

[hafriedlander]

What if instead of registering the app namespace globally, each of these modules required the user inform them of the app namespace, e.g. Blog:set_page_namespace(\UncleCheese\Mysite');.

You can't do a dynamic extend call (i.e. class BlogPost extends $namespace\Page doesn't work). The POC I put up uses the autoloader to dynamically replace __App\Page with MySite\Page or whatever, but the autoloader has no context for what class is doing the extending. So you'd need to use extends __App\ForBlogPost\Page or some other terrible incantation that the autoloader string-parsed out.

[sminnee]

Speaking of smooth upgrade paths :) Isn't Page a special case, as it's by default installed into the mysite/code/ directory? That being the case, why do anything here? It's in the global namespace, always is and why can't it continue to be?

That was one of the options. It seemed weird to have 1 class that was the exception, but it is the "zero extra coding" option and if we were planning on deprecating it's special use in the future, maybe it's not such a bad choice.

[unclecheese]

I like @patricknelson 's suggestion:

namespace \MyApp\Pages;

class Page extends \Framework\SiteTree {}

// Fix modules
class Page extends \MyApp\Pages\Page

[sminnee]

Or

class_alias('MyApp\Pages\Page', 'Page');
class_alias('MyApp\Pages\Page_Controller', 'Page_Controller');

UPDATE: Fixed code

[hafriedlander]

Other way around Sam ($original, $alias). But yeah, that'd work. Don't forget Page_Controller too.

[sminnee]

Which brings us full circle, but instead of treating the __App namespace as special, we're treating the Page class as special. Since this problem exists with only 2 classes, perhaps a more narrowly applied fix is better.

[patricknelson]

Yeah. Think about the alternatives. I've tended have these two rules of thumb which may apply to this situation:

1. A simpler solution which meets all requirements is a superior one.
2. If you have to resort to hackery or magic :rabbit::tophat: it's a possible indicator that you might be "doing it wrong."

And re: @hafriedlander

So you'd need to use extends __App\ForBlogPost\Page or some other terrible incantation

My point exactly. The less of this, the better. What's so horrible about just leaving it at \Page (or some other standardized/known static-and-not-variable namespace) and letting that be a sort of standard baseline proxy for gluing things together?

Also @sminnee other than Page which needs this, what's the other class?

[hafriedlander]

The advantage of App is: less chance of a class name clash, able to "redefine" what App points to (if two different places try and call class_alias you'll get a runtime error), only needs defining once (rather than twice for Page and Page_Controller) The advantage of raw class_alias is: simpler, less magic.

Both require changes to modules (Page => \Page or \__App\Page). Both have bootstrap races (referencing Page before the config setup).

@patricknelson: extra class is Page_Controller.

[sminnee]

Both require changes to modules (Page => \Page or \__App\Page).

Only at the point that modules put their code into namespaces. An unnamespaced module will can keep referencing "Page" and the need to switch to "\Page" is obvious.

Both have bootstrap races (referencing Page before the config setup).

Only at the point that modules put their code into namespaces. An unnamespaced project can just define Page directly.

If we're expecting that by SS5 the subclassing of Page will be deprecated, perhaps a narrower/simpler solution is best?

[patricknelson]

Also, even if we had a bootstrap race of some sort, why not just rely on composer as a standard for not only defining and autoloading our PSR-4 namespaces for cms/ and framework/ but also encourage developers to utilize standard processes such as defining an autoload/classmap entry in composer.json if they have issues? e.g.

    "classmap": [
        "mysite/code/Some/Namespace/Page.php"
    ]

This would "just work" and doesn't require any extra SilverStripe-specific magic/abstraction.

[hafriedlander]

Be careful with classmap. Unlike with class_alias, \Page and \Some\Namespace\Page will be different classes (so different config, static state, etc).

[flamerohr]

I'd be more in favour of Alternative 1 and set __App as something by default in the silverstripe-installer?

Although moving modules away from the Page dependency would be nice, I could see this work being quite a big task to do.

[unclecheese]

I'm not sure I agree that class_alias is less magic. Maybe in this case, but to me, class_alias seems opaque, confusing and magical. It feels like an anti-pattern in general PHP coding, perhaps even a last resort hack, and might set a bad precedent for developers learning PHP by example in SilverStripe.

If you don't know the class_alias PHP feature exists, you are going to have a hard time figuring out where the implementation of \__App\Page actually comes from in a codebase you inherit. Whereas simply searching for class Page, or class \Page would be sufficient to find the other approach.

Simplicity is not brevity. To me, simplicity means something is easy to understand, has few working parts, it's transparent, and above all common. My worry with the class_alias approach is that junior PHP devs just starting out would get the impression that it's best practice, and I think we all agree that this isn't.

[sminnee]

@unclecheese I'm not sure which approach you are advocating for now. Can you clarify?

[unclecheese]

Per my previous comment, I still think the approach detailed by @patricknelson makes the most sense. It just feels the most honest to me, for lack of a better word. We're building an escape hatch, not a long-term solution, so my feeling is that it should look like one.

[unclecheese]

Perhaps "bridge" is a better term than "escape hatch," but the point being, we're painted in a corner, and we need a reasonable path to moving forward without completely hosing thirdparty code. Whatever we choose is going to be ugly, so let's not try to paint over it to make a hack look pretty.

[patricknelson]

Conceptually doesn't even have to be a escape hatch per se. For the lack of a better metaphor, it's more of a central proxy or funnel or gluing framework/cms code to the developer's code. It's intuitive and, if you need to workaround it, you simply need to know a little PHP and you're [hopefully] set.

Edit: Yep - bridge is another good metaphor :smile:

[sminnee]

I generally agree with using the non-namespaced Page as the default value. My only quibble is whether we do this:

class Page extends \MyApp\Pages\Page {}
class Page_Controller extends \MyApp\Pages\Page_Controller {}

Or this:

class_alias('MyApp\Pages\Page', 'Page');
class_alias('MyApp\Pages\Page_Controller', 'Page_Controller');

The former is going to generate more cruft that we have do deal with. Off the top of my head:

• Additional ClassName values in the enum
• Additional page types to select from the dropdown, or be disabled (we'd need to replace the hide_ancestor or need_permission semantics with something new to make this possible, I believe)
• Additional template paths that are searched for (both templates/MyApp/Pages/Page.ss and templates/Page.ss will be searched for)

In short, we'd introduce a lot of weight that isn't brought in by class_alias(). I can't see how we're helping newcomers in advocating for that.

If we went with the first example, would we be asking developers to write these subclasses themselves, or to have some kind of class generator?

The one thing that the former (explicit subclass) option has going for it is that it doesn't require code modifications: we can start using it on some sample projects and seeing what happens.

On the escape hatch metaphor: this is one reason why I like the idea of it being a composer micropackage that provides a plugin to its autoloader: it can sit at arms length from the rest of the framework, and be easily removed if & when it's no longer needed. That said, I'm not sure how easy it is to write plugins for the composer autoloader... :-/

[sminnee]

On the other side of the coin, using class_alias() isn't inspected by the class manifest, so using that would require additional modifications to that code.

[patricknelson]

Maybe I could counter a couple points, at least:

Additional ClassName values in the enum

Personally I don't see this as an issue. That's a machine problem, we're here to make developers' lives easier :sunglasses:

Additional page types to select from the dropdown, or be disabled (we'd need to replace the hide_ancestor or need_permission semantics with something new to make this possible, I believe)

HiddenClass can be used/applied to these classes (if a developer needed to fix issues), however I see the benefit of class_alias() here due to the existing magic already in place and how I'd continue with favoring @unclecheese's approach here to help abstract that cruft away from our plush developers' eyes in case our proxy/bridge solution goes that direction.

Additional template paths that are searched for (both templates/MyApp/Pages/Page.ss and templates/Page.ss will be searched for)

I could be mistaken (from reading the code) but it looks like the manifest is generated just by looking at what's already on the file system (regardless of if it's ever used) and then once a template is requested, a match from this manifest is then retrieved (i.e. no additional file overhead nor CPU/array access, etc). In other words, seeing up an additional class shouldn't slow anything down in this regard. I think...

[sminnee]

I'd continue with favoring @unclecheese's approach here to help abstract that cruft away from our plush developers' eyes in case our proxy/bridge solution goes that direction.

@patricknelson @unclecheese the comments that you reference talk about a number of topics, and so I don't know if what you're referring to is module-specific class-remapping, but if so: isn't feasible for the reasons that Hamish has elaborated on.

I'm going to update the original post with some amended options based on the discussion.

[sminnee]

Ok, can we get some votes on either this comment or the two following:

I VOTE FOR OPTION 20160729-A Use class_alias to map to re-map Page and Page_Controller.

[sminnee]

I VOTE FOR OPTION 20160729-B Use an explicit sub-class to re-map Page and Page_Controller.

[sminnee]

I REJECT BOTH 20160729-A and 20160729-B and want to keep discussing alternatives.

[dhensby]

These solutions seem nasty to me.

I think I lean towards keeping Page in the global namespace - if devs want to namespace it, then they can do 20160729-B themselves.

If we're talking about removing this in SS5 then all of these involve work that is to be scrapped later, so why not pick the option with no additional work?

[patricknelson]

I agree @dhensby. Also, correct me if I'm wrong, but I think much of the voting here is to take more of a formal/official approach on not only technically what we implement but also in what we recommend to developers should they decide to namespace all of their pages.

By the way, what's on the horizon for v5 with respect to this? Sounds like it'd be a pretty big shift in approach (not necessarily bad).

[riddler7]

What about overwriting the default constructor for Page and adding a deprecation notice?

[tractorcow]

I think the advantage of -B is that this behaviour is simply a recommendation, rather than a requirement. It's already possible to follow this pattern in a 3.x project.

As far as the former solution -A is concerned, this again has no effect on modules, as Page is still the default page alias.

I don't see any disadvantage to simply supporting both styles, yet not enforcing either by default in the installer project, but instead documenting the various ways in which "How you can namespace your project / Page class", thus leaving it up to the preference of the developer.

In order to address the issue of "race conditions" in bootstrapping of a class alias, I suggest to document the various ways that this can be achieved via composer configuration (of which there are a variety of options).

[tractorcow]

I voted the third option with "support both, enforce neither, and document options" as the "alternate solution". :P

[micmania1]

I'm with @dhensby. Its simple, does the job and requires the least changes to core and modules.

[sminnee]

I think I lean towards keeping Page in the global namespace - if devs want to namespace it, then they can do 20160729-B themselves.

Sorry if that's not clear @dhensby and @tractorcow, that's what I meant! I'm going to re-word the descriptions to clarify.

UPDATE: Note that we'd still need to implement hide_pagetype as a core change, but that would be the limit of the core changes needed.

[sminnee]

By the way, what's on the horizon for v5 with respect to this? Sounds like it'd be a pretty big shift in approach (not necessarily bad).

Most likely, we'd shift from page types to content blocks as being the primary way in which modules provided client-site customisations. So instead of a class BlogPage extends Page or class UserDefinedForm extends Page you'd have subclasses of Block or Widget or something.

We'd have to get agreement that this is a path we want to go down, choose one of the umpteen content block systems to upgrade to a supported module / core feature, add any features to it that we can't live without, update our recommendations to module maintainers, and get modules changed. Doing this before SS4 is released seems unrealistic.

I expect that we'll do some of these steps during SS4's lifetime rather than in the lead-up to it.

[sminnee]

I voted the third option with "support both, enforce neither, and document options" as the "alternate solution". :P

Just so you're aware @tractorcow, this means writing class_alias support into the manifest, and implementing hide_pagetype. That's the reason I didn't vote for this option.

[sminnee]

I agree @dhensby. Also, correct me if I'm wrong, but I think much of the voting here is to take more of a formal/official approach on not only technically what we implement but also in what we recommend to developers should they decide to namespace all of their pages.

Yeah, the recommendation would go into the docs under a heading "how do I add namespaces to my project code?"

[dhensby]

Sorry if that's not clear @dhensby and @tractorcow, that's what I meant! I'm going to re-word the descriptions to clarify.

Ah, so which should I be voting for? ;)

---

Most likely, we'd shift from page types to content blocks as being the primary way in which modules provided client-site customisations. So instead of a class BlogPage extends Page or class UserDefinedForm extends Page you'd have subclasses of Block or Widget or something.

This would be nice, and page templates could be more like definitions of layout zones / default block arrangements.

[patricknelson]

Re: @riddler7's comment here, just to be thorough:

What about overwriting the default constructor for Page and adding a deprecation notice?

I should hope that's certainly not in the cards at the moment.

Also re: @sminnee's comment, I'm glad I asked:

Most likely, we'd shift from page types to content blocks as being the primary way in which modules provided client-site customisations. So instead of a class BlogPage extends Page or class UserDefinedForm extends Page you'd have subclasses of Block or Widget or something.

I'd look forward to a spec/RFC on that :sunglasses: Not sure what to make if it yet so I'm going to stay silent on that topic.

[sminnee]

Ah, so which should I be voting for? ;)

@dhensby vote for Option 20160729-B :-)

[dhensby]

OK - so basically I vote -B but with this approach:

class Page extends \MyApp\Pages\Page {}
class Page_Controller extends \MyApp\Pages\Page_Controller {}

Basically, doing this would be optional if the dev decided they wanted their Page class to be namespaced.

[sminnee]

They would need to set $hide_pagetype = true on it as well, otherwise you'll get double ups in the page type selector, but yes this shim is only necessary if you decide to namespace your Page class

[chillu]

Back from holidays, phew a lot of good stuff to catch up on! One factor which (I think) hasn't been mentioned: class_alias() breaks IDE support for autocomplete on ancestors (PHPStorm issue). This is particularly damaging on a central class like Page/SiteTree in the SilverStripe context. Laravel Facades have the same issue, which you can fix by a Laravel specific IDE hint generator module - hardly ideal.

I've voted for -B (let devs optionally use subclasses), with the minimum necessary core dev work to allow for this ($hide_pagetype?). Namespaces are great, but if they affect how devs can build mental models of how SilverStripe works (through magic class maps), it's a step too far.

Unless devs implement this workaround described in -B, the template folder structure will be confusing by default, right? In my opinion that's the biggest downside. The mixed messages about namespacing your own code might turn off devs from using namespaces in SS4. I don't have a better answer though (other than the shift to a more content block focused model).

mysite/
  templates/
    Layout/
      Page.ss
    MyAppNamespace/
      Layout/
        MyPage.ss

[Firesphere]

Slightly related. With my PR in the CMS here: https://github.com/silverstripe/silverstripe-cms/pull/1641 it is also possible to do the following in YML and have no root Page class:

SilverStripe\Core\Injector\Injector:
  Page:
    class: My\Namespaced\Class\Page

Creating multiple options for the dev as a starting point.