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

PagesType::getPageClass not working correctly when PageClass is namespaced #1222

Closed
sebastiandittrich opened this issue Aug 3, 2020 · 6 comments

Comments

@sebastiandittrich
Copy link

Short description of the issue

The PagesType::getPageClass method should return the class that was set previously with PagesType::setPageClass. But it returns an unnamespaced version of the classname.

I have the following setup:

<?php namespace ProcessWire\MyModule\PageTypes;

class Bookings extends \ProcessWire\PagesType
{
    public function __construct(/* ... */)
    {
        parent::__construct(/* ... */);
        $this->addTemplates('service');
        $this->setPageClass(ProcessWire\MyModule\Pages\Booking::class);
    }
}

// Later in MyModule.module.php
$this->bookings = new Bookings(/* ... */);

Expected behavior

$mymodule->bookings->getPageClass();
// => ProcessWire\MyModule\Pages\Booking

Actual behavior

$mymodule->bookings->getPageClass();
// => Booking

Possible Fix

Change false to true in this line: https://github.com/processwire/processwire/blob/master/wire/core/PagesType.php#L607

Steps to reproduce the issue

  1. Create a Page class in different Namespace than ProcessWire\
  2. Create a PagesType class calling setPageClass in the constructor with the created Page class.
  3. Try to retrieve a Page using PagesType::get(...)
  4. Note that the returned class is not an instance of the created Page class

Setup/Environment

Server Details

Software Version
ProcessWire 3.0.164
PHP 7.4.8
Webserver Apache/2.4.35 (Win64) OpenSSL/1.1.1g mod_fcgid/2.3.9
MySQL Server 5.7.24
MySQL Client mysqlnd 7.4.8
Server Settings
Parameter Value
allow_url_fopen 1
max_execution_time 36000 (changeable)
max_input_nesting_level 64
max_input_time 60
max_input_vars 1000
memory_limit 512M
post_max_size 2G
upload_max_filesize 2G
xdebug
xdebug.max_nesting_level
mod_rewrite 1
mod_security
EXIF Support
FreeType 1
GD Settings
Parameter Value
Version bundled (2.1.0 compatible)
GIF 1
JPG 1
PNG 1
WebP 1
Module Details
Module ClassName Version
BookingStudio 1.0.0
FieldtypeMystique 0.0.15
InputfieldMystique 0.0.15
Mystique 0.0.15
ProcessTracyAdminer 1.0.7
TracyDebugger 4.21.20
@ryancramerdesign
Copy link
Member

@sebastiandittrich ProcessWire doesn't currently support Page classes in other namespaces. Though I do think it's likely something we can support at some point if there's a strong need for it. But there may be more drawbacks (especially when it comes to hooks) than benefits at this stage, though I don't know for sure... I've never had a quantity or ambiguity of Page classes where I'd need to start introducing levels of namespaces to them. For your case, since you are already creating a PagesType class, I think you could achieve the same result as your suggested possible fix by overriding the getPageClass method:

public function getPageClass() {
  return "\ProcessWire\MyModule\Pages\Booking::class";
}

While I'm suggesting that as an alternative to what you mentioned, I do want to reiterate that PW just isn't designed for Page classes in some other namespace, so I have no idea how far you can get it doing it. I would be curious what you find though. Thanks.

@sebastiandittrich
Copy link
Author

But there may be more drawbacks (especially when it comes to hooks) than benefits at this stage, though I don't know for sure...

I am working with namespaced custom page classes for a while now, but and I haven't encountered any issues with hooks or anything else. This issue is the only point that caused problems. Hooks are anyway not aware of namespaces, as stated here: #109.

I've never had a quantity or ambiguity of Page classes where I'd need to start introducing levels of namespaces to them.

I personally don't think that you only need namespaces if you have many pages, but in general when you have a lot of code. Even if there is only one custom page class, I think it makes sense to namespace it. It just gives your code a clean structure and prevents possible naming collisions especially if you are developing a module.

For your case, since you are already creating a PagesType class, I think you could achieve the same result as your suggested possible fix by overriding the getPageClass method:

Yes, thanks, this is a valid workaround. But it would be cool if there was native support for namespaced pages in ProcessWire, that's why I've opened this issue.

Thank you for your response!

@ryancramerdesign
Copy link
Member

Glad that override suits your need. It's not technically even a workaround since getPageClass() is a template method, designed to be overridden by a class extending PagesType. So if you find it works, consider it a permanent solution as that method will always be there. I'm going to close this for now since the base PagesType class is working as intended, but if the need arises more feel free to open in the requests repo.

@BernhardBaumrock
Copy link

Just wanted to mention that I'm using custom namespaces for custom pageclasses all the time. So far no issues. I'm setting the pageclass via RockMigrations:

<?php namespace RockMailer;

use ProcessWire\HookEvent;
use ProcessWire\Page;

class Contact extends Page {

  [...]

  /**
   * Migrate this pageclass
   */
  public function migrate() {
    $mailer = $this->wire->rockmailer; /** @var RockMailer $mailer */
    $rm = $mailer->rm();
    $rm->migrate([
      'templates' => [
        self::tpl => [
          'tags' => self::tags,
          'icon' => 'user-o',
          'noSettings' => 1,
          'pageClass' => '\RockMailer\Contact',
          'fields' => [
            'title',
            // channel fields are added below
          ],
        ],
      ],
    ]);

    [...]
  }

}

And I get properly namespaced page classes:
img

On hooks I use the page template or instanceof to check if the hook applies or not.

Was not aware that page classes are not designed for custom namespaces... I hope I'm not running into troubles...

@ryancramerdesign
Copy link
Member

@BernhardBaumrock If you are overriding the method or doing this with the API, and you find everything continues to work how you want it, then I don't think you'll run into any troubles later. I hadn't tested it myself and so am not as well prepared to provide technical support for it, but I also see no reason why it shouldn't work. If you find it works well then I think we can say that we support it. It may be that I just need to look at the input side in the template editor system tab to make sure namespaces aren't getting sanitized out or something.

@BernhardBaumrock
Copy link

thx @ryancramerdesign , I missed your reply until now... just wanted to support @sebastiandittrich 's statement about using namespaced page classes! I'm using it all the time - it makes all the code so much cleaner, so please have that in mind for whatever you do with the core in the future :)

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

No branches or pull requests

3 participants