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

Pages for process modules don't have a title in specific situations #1462

Closed
MoritzLost opened this issue Oct 28, 2021 · 9 comments
Closed

Comments

@MoritzLost
Copy link

Short description of the issue

This is a follow-up to this forum thread. Here's a summary of the issue:

I have a problem with Process modules that define a custom page using the page key through getModuleInfo. Those pages are created automatically when the module is installed. The problem is that the title of the page only gets set in the current language. That's not a problem if the current language (language of the superuser who is installing the module) is the default language; if it isn't, the Process page is missing a title in the default language. This has the very awkward effect that a user using the backend in the default language (or any other language) will see an empty entry in the setup menu.

Expected behavior

When installing a Process module with a page key, ProcessWire should set the title of the generated page in all languages (or at least in the default language), regardless of the account language of the user installing the module.

Actual behavior

The process page only get's a title in the account language of the user installing the module, which may not be the default language.

Screenshots/Links that demonstrate the issue

Screenshot of a process module page with an empty title in the setup menu:

1317970094_Screenshot2020-05-12at15_34_17 png 872136a1bae40e59de313ed2d8654eac

Suggestion for a possible fix

Upon installation, ProcessWire should check if the site has multiple languages and set the title in each language automatically.

Steps to reproduce the issue

  1. Create a ProcessWire installation with two languages: English (default) and German (german).
  2. Set your user account to German.
  3. Install the ProcessCacheControl module.
  4. Now change your account language to English. Log out and log back in again (this clears the navigation cache).
  5. Mouseover the Setup menu – the Cache Control entry (with the floppy disk icon) will have an empty title as seen in the screenshot above.

Setup/Environment

ProcessWire: 3.0.185
PHP: 7.4.25

@ryancramerdesign
Copy link
Member

@MoritzLost I can't seem to duplicate the issue here. When I install any module that specifies the 'page' property in its getModuleInfo, and I'm in a language other than default, the created page still gets its default language populated. This is because ProcessWire's module installer sets the language to default before it calls a module's ___install() method. Something I can spot in that module you mentioned is that it's not extending the Process classes ___install() method because it lacks the 3 underscores at the beginning of the method name, so I'm thinking that it may be bypassing at least part of the built in logic for automatic page creation. It should also be calling parent::___install() rather than parent::install() since there is no parent::install() method. If you update the method name in that module to ___install does that fix it?

@MoritzLost
Copy link
Author

MoritzLost commented Nov 4, 2021

@ryancramerdesign Thanks for the reply!
I've tested your suggestion but it doesn't appear to make a difference. To test this, I've uninstalled the module and then updated the install method like you suggested:

    /**
     * Called by ProcessWire during module installation.
     *
     * @return void
     */
    public function ___install()
    {
        // save an installation message to force creation of the cache-control log file if it doesnt exist yet
        $tools = $this->modules->get('CacheControlTools');
        $tools->logMessage($this->_('ProcessCacheControl installed successfully. Creating the dedicated log file.'));
        return parent::___install();
    }

But it's still not working correctly. In my test setup, there are two languages: German (default) and English (english). My account is set to English. When I install the module (with the updated ___install method above), the title of the process page is still only populated in English:

Cache Control page with missing title in default language

Could you take another look? Any other implementation errors in my module that might be causing this?

@matjazpotocnik
Copy link
Collaborator

@MoritzLost can you replicate the issue if you comment out the install() method in /site/modules/ProcessCacheControl/ProcessCacheControl.module in line 264? Also, don't use ___install() method.

ryancramerdesign added a commit to processwire/processwire that referenced this issue Aug 17, 2023
@ryancramerdesign
Copy link
Member

@MoritzLost @matjazpotocnik I was able to duplicate this with ProcessCacheControl. While the install method should ideally be named ___install() and call parent::___install() rather than parent::install(), I don't think that's the source of the issue. I think the source of the issue is that the base Process::___install() method didn't consider language, so I have pushed a fix for that. Now it enforces default language when creating the new page, since other languages will inherit the default language value. But default won't inherit from some other language, which is why it was blank before.

@matjazpotocnik
Copy link
Collaborator

Ryan, I asked Moritz to test with a reason: I can always replicate the issue IF this method is "active":

    public function install()
    {
        // save an installation message to force creation of the cache-control log file if it doesnt exist yet
        $tools = $this->modules->get('CacheControlTools');
        $tools->logMessage($this->_('ProcessCacheControl installed successfully. Creating the dedicated log file.'));
        return parent::install();
    }

When I comment install() method, the issue is gone. I didn't dig further as I first wanted a confirmation of my findings.

Now, with your fix in place, it works regardless of the presence of the install() method.

@matjazpotocnik
Copy link
Collaborator

@MoritzLost I confirmed Ryan's fix is working so I'm closing this issue. Please reopen if needed.

@MoritzLost
Copy link
Author

@matjazpotocnik @ryancramerdesign Sorry for the late reply, I was out sick for a couple of weeks. Thanks for the fix!

So should I still adjust my module? I.e. change the install method to ___install and then call parent::___install instead of parent::install? If so, could you elaborate why this is necessary? I thought that if I use parent::install(), this would invoke the magic method which should allow hooks to run correctly. Is this incorrect?

@ryancramerdesign
Copy link
Member

@MoritzLost If your module extends another than you want to use whatever the parent module is doing. So if the parent module defines an ___install() method then you should also define yours as ___install() and you should call parent::___install() within it (if you need to call the parent method). But if the parent module defines install() then remove all of the ___ from the prior examples.

The reason why you would want to call parent::___install() is so that any hooks to that method don't get called twice. The same is true of any Wire-extending class: if it overrides a hookable method, then its parent call (if used) should be parent::___method(). That way any hooks to the method only get called once.

If the module does not extend another, then you can choose whether you want to implement install() or ___install(). I'm not sure it matters, as I don't know of anyone hooking module install methods, but I usually go with ___install() just in case. :)

@MoritzLost
Copy link
Author

@ryancramerdesign Thanks for the explanation. I've updated my module accordingly: https://github.com/MoritzLost/ProcessCacheControl/releases/tag/1.1.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants