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 😄 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 🐰🎩 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 😄

[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 😎

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.

[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: silverstripe/silverstripe-cms#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.

[patricknelson]

@Firesphere Does that still allow you to do this?

<?php
namespace My\Namespaced\Class

class Page extends \Page {}

Ultimately I believe your custom namespaced Page instance must be a child of \SiteTree (not 100% sure about v4 these days, however, but that's likely still true).

[sminnee]

@Firesphere it won't work

[colintucker]

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

Just thought I'd add my two cents here and mention that it doesn't seem to actually work if you add the classes to _config.php, at least not for me in the latest SS4 dev-master. Whether it's something to do with the manifest or not, I'm not sure.

The classes are exposed to PHP (i.e. class_exists('Page') returns true), but SS doesn't seem to recognise them, and you'll end up with problems such as the dev/build failing with an Invalid default value for 'ClassName' error.

EDIT: yep the manifest doesn't look at _config.php. I realise though that "include it in their _config.php" may be a bit ambiguous... including the classes there will not work, doing an include() on the helper file however should work.

[kinglozzer]

Just thought I’d drop by to add the method we’re using to overcome this. We have App\Model\Page, which all our custom page classes extend, and then provide the following “polyfill” for other modules that require \Page and \PageController.

bootstrap.php (probably to be renamed on reflection), included in composer’s autoloader:

<?php

use App\Control\PageController as NamespacedPageController;
use App\Model\Page as NamespacedPage;

class Page extends NamespacedPage
{
    private static $hide_ancestor = NamespacedPage::class;
    private static $table_name = 'SilverStripe_Page';
}

class PageController extends NamespacedPageController
{
}

Using hide_ancestor means that all pages created in the CMS will be \Page, rather than App\Model\Page, but we’ve not found that to cause any issues - it inherits everything we add to our namespaced page/controller anyway, and makes no difference to the CMS users.

[rhysstubbs]

Firstly, apologies for commenting on what seems to be an issue that has been sorted and referenced multiple times! I have recently upgraded a project to V4 and decided to namespace all of the application code. I ran into the issue Class Page not found... and landed here and all has been fine until today. Initially I decided to not namespace the Page and PageController classes and then extend those when needed.

However, today I have run into a problem regarding "generic" page types. For some context here is a snapshot of my template and src directory structure ():

src/
    controllers/
        PageController.php
        *.php
    models/
        Page.php
        *.php

templates/
    Page.ss
    Includes/
    Namespace/
        Web/
            Layout/
                Page.php
                *.php

I followed the advice given in this thread and created the two classes to sit in the global namespace which I can include in _config.php. When I try and load the application I am faced with Fatal error: Class 'Page' not found in /**/*/ErrorPage.php on line 37. At this point I am feeling rather stupid, is there additional documentation that I have missed or is this no-longer the supported workaround for the latest release of v4?

Here is the helper file I am including in _config.php

use Namespace\Web as App;

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

class PageController extends App\PageController
{ }

UPDATE:

I think another error was causing my issue previously and I now believe I have now gotten the above working. But I now get

Uncaught LogicException: Multiple classes ("Namespace\Web\Page", "Page") map to the same table: "Page"

Which makes sense. It'd be much nicer if the globally name-spaced Page could be abstract as I don't actually care for it outside of inheriting from it. Is there something I've missed (again)? I don't want two different "Page" class types in the code base (C# partials would be great here!). If I move all of the methods and properties from the name-spaced Page into the global Page I am back at square one again and may as well delete the Namespace\Web\Page?

The initial issue that got me into writing this comment was that the generic pages on the site were using the Page type in the global namespace thus rendering the wrong template since there is two Page.ss files in the templates directory. Creating new generic pages in the CMS using the "Page" type and not the Namespace\Web\Page type.

Also, one question RE the proposed solution... why is it that the globally name-spaced Page inherits from the name-spaced version instead of the the opposite?

[torleif]

Warning regarding @kinglozzer fix for getting extensions working that reference the base Page class. It breaks the history page in the CMS

[kinglozzer]

@torleif That’s weird, page history still works perfectly for us

EDIT: I was wrong, it doesn’t work. It works for Page directly, but not for any other pagetypes as they extend App\Model\Page instead of Page

[torleif]

@kinglozzer History will fail to show for all pages that extend from Page. If you extend from \MyNameSpace\Page, you'll be ok. I suspect this is because ReadOne.php in the GraphQL resolver will try to find all classes that extend from the base page hierarchy. We had the issue of many of our pages not having history, as we're running several extensions which extend from Page. Some extensions don't have the option of modifying the code, so I worked on this issue over the last day and came up with a fix that will allow all pages to show up in the history tab:

app/_config/config.yml

SilverStripe\GraphQL\Scaffolding\Scaffolders\CRUD\ReadOne:
  extensions:
    - OP\ReadOneExtension

app/src/extensions/ReadOneExtension.php

<?php

/**
 * Fix for allowing graphQL quries to select from items that extend from Page
 * Note the namespace in \OP\Page. Replace with your own namespace
 * @author torleif west <torleifw@op.ac.nz>
 */

namespace OP;

use SilverStripe\Core\Extension;
use SilverStripe\ORM\DataList;

class ReadOneExtension extends Extension
{

    /**
     * updates the list with the correct namespace
     * @param DataList $list
     * @param array    $args
     * @param mixed    $context
     * @param mixed    $info
     * @return $void
     */
    public function updateList(DataList &$list, $args, $context, $info)
    {
        if ($list->dataClass() == 'Page') {
            $list = DataList::create('\OP\Page');

            if (isset($args['ID'])) {
                $list = $list->filter('ID', $args['ID']);
            }
            if ($this->owner->queryFilter() && $this->owner->queryFilter()->exists()) {
                $list = $this->owner->queryFilter()->applyArgsToList($list, $args);
            }
        }
    }
}

I'll try some testing on a fresh install to see if i can replicate the issue with clean code.

Edit: I managed to replicate this issue on a fresh CWP install

[GuySartorelli]

Given that the RFC itself is about the namespace of Page and mentions an option as accepted which doesn't require any changes to be made to core code, I am going to close this issue.
If there are outstanding discussions about things that don't work as expected they should be raised as their own issues which can be triaged and dealt with on their own merits.