luyadev / luya-module-cms

The LUYA CMS module provides a full functional CMS for adding contents based on blocks.
https://luya.io
MIT License
33 stars 46 forks source link

Env option pageObject is incomplete in inactive page versions #414

Closed hbugdoll closed 8 months ago

hbugdoll commented 8 months ago

What steps will reproduce the problem?

What is the expected result?

What do you get instead?

Edit: The issue is not only related to new page versions, but to inactive page versions.

Additional infos

Q A
LUYA Version Admin 5.0, CMS 5.1
PHP Version 8.2
Platform Apache
Operating system Linux Server
hbugdoll commented 8 months ago

Ok, the reason is clear. https://github.com/luyadev/luya-module-cms/blob/496cebc789658537381825958f8ae72524b4f6e7/src/base/NavItemType.php#L33-L36 returns the navItem only for the active page version (with nav_item_type_id).

hbugdoll commented 8 months ago

I tried

public function getNavItem() 
{ 
    return $this->hasOne(NavItem::class, ['id' => 'nav_item_id'])->where(['nav_item_type' => static::getNummericType()]);
} 

in the manner hasOne(..., PK => FK) like in NavItemPage::getForceNavItem(). It works and unit tests are running successfully.

@nadar If you agree, I could provide a PR.

hbugdoll commented 8 months ago

@nadar If you agree, I could provide a PR.

I am now convinced that this must be fixed.

nadar commented 8 months ago

But why not use getEnvOption('pageObject')->forcedNavItem instead of getEnvOption('pageObject')->navItem?

The getEnvOption('pageObject') is a misleading concept anyhow, but introduced back in the days. It would be so much better to have $this->page-><NavItemPage> so the IDE knows all possible methods to use, instead of array keys you have to lookup on guides. But thats another story :-)

hbugdoll commented 8 months ago

Thanks @nadar for reply in busy times.

But why not use getEnvOption('pageObject')->forcedNavItem instead of getEnvOption('pageObject')->navItem?

Yes, that would be a possibility in my case. But in respect of the absent type check in getEnvOption('pageObject')->forcedNavItem I would prefer the straightforward solution...

The getEnvOption('pageObject') is a misleading concept anyhow, but introduced back in the days. It would be so much better to have $this->page-><NavItemPage> so the IDE knows all possible methods to use, instead of array keys you have to lookup on guides. But thats another story :-)

Yeah, indeed!

hbugdoll commented 7 months ago

The getEnvOption('pageObject') is a misleading concept anyhow, but introduced back in the days. It would be so much better to have $this->page-><NavItemPage> so the IDE knows all possible methods to use, instead of array keys you have to lookup on guides. But thats another story :-)

Ok, let's go 🚀