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

AbstractBundle - getBasePath - missing last letter #3862

Closed
Kaik opened this Issue Nov 27, 2017 · 15 comments

Comments

Projects
None yet
3 participants
@Kaik
Copy link
Contributor

Kaik commented Nov 27, 2017

Q A
Zikula Version 1.5.x
PHP Version 7x

Expected behavior

full base path

Actual behavior

missing the last letter in the base path

Steps to reproduce

dump($this->kernel->getModule('ZikulaUsersModule')->getBasePath());

"/home/Kaik/public_html/core15/src/system/UsersModul"

I do not know if it is on purpose or OS dependent. I just noticed it while working on Dizkus and eventually used getNamespace.

@craigh

This comment has been minimized.

Copy link
Member

craigh commented Dec 13, 2017

what are these results?

dump($this->kernel->getModule('ZikulaUsersModule')-> getNamespace());

dump($this->kernel->getModule('ZikulaUsersModule')-> getPath());

@Kaik

This comment has been minimized.

Copy link
Contributor

Kaik commented Dec 13, 2017

In controller: (core 1.5.2 and 2.0.3)

dump($this->get('kernel')->getModule('ZikulaUsersModule')->getBasePath());
result:
"/home/Kaik/public_html/mojeleicester/src/system/UsersModul"

dump($this->get('kernel')->getModule('ZikulaUsersModule')->getNamespace());
result:
"Zikula\UsersModule"

dump($this->get('kernel')->getModule('ZikulaUsersModule')->getPath());
result:
"/home/Kaik/public_html/mojeleicester/src/system/UsersModule"

As I wrote before I'm using getNamespace it works and it fits better for what I need, but during testing those methods getBasePath missing last letter brought my attention.

@Guite Guite added the Bug label Dec 13, 2017

@Guite Guite added this to the 1.5.4 milestone Dec 13, 2017

@Guite

This comment has been minimized.

Copy link
Member

Guite commented Dec 13, 2017

@Kaik can you please try
replacing https://github.com/zikula/core/blob/1.5/src/lib/Zikula/Core/AbstractBundle.php#L148
by:

$this->basePath = substr($path, 0, strrpos($path, $ns));
$this->basePath = rtrim($this->basePath, '/');

Does this fix the problem?

@Kaik

This comment has been minimized.

Copy link
Contributor

Kaik commented Dec 13, 2017

@Guite no luck, unfortunately:
Method after changes looks like this hope this is what you meant

    public function getBasePath()
    {
        if (null === $this->basePath) {
            $ns = str_replace('\\', '/', $this->getNamespace());
            $path = str_replace('\\', '/', $this->getPath());
            $this->basePath = substr($path, 0, strrpos($path, $ns));
	    $this->basePath = rtrim($this->basePath, '/');
        }

        return $this->basePath;
    }

And the result is an empty string "". I have tried onliner as well - my ide is complaining when I do assign variable twice but $this->basePath = rtrim(substr($path, 0, strrpos($path, $ns)), '/'); returns exactly the same result an empty string.

@Guite

This comment has been minimized.

Copy link
Member

Guite commented Dec 13, 2017

How about this:

$basePath = substr($path, 0, strrpos($path, $ns));
$this->basePath = substr($basePath, -1) == '/' ? substr($basePath, 0, -1) : $basePath;

?

@Kaik

This comment has been minimized.

Copy link
Contributor

Kaik commented Dec 13, 2017

Empty string.
Maybe those bits will help

    public function getBasePath()
    {
        if (null === $this->basePath) {
            $ns = str_replace('\\', '/', $this->getNamespace());
            $path = str_replace('\\', '/', $this->getPath());
            $basePath = substr($path, 0, strrpos($path, $ns));
	    dump($path); gives "/home/Kaik/public_html/core15/src/system/UsersModule"
	    dump($ns); gives "Zikula/UsersModule"
	    $this->basePath = substr($basePath, -1) == '/' ? substr($basePath, 0, -1) : $basePath;
        }

        return $this->basePath;
    }
@Guite

This comment has been minimized.

Copy link
Member

Guite commented Dec 14, 2017

Seems like this whole getBasePath method is not solid because it assumes the namespace is part of the path.
Honestly I would like to deprecate and remove it (looks like it isn't used anywhere in the core, too).

What exactly do you expect from it? /home/Kaik/public_html/core15/src/system/?

@craigh opinion?

@craigh

This comment has been minimized.

Copy link
Member

craigh commented Dec 14, 2017

I agree it is probably no longer useful. It was probably used in PSR-0 days, but now we are requiring PSR-4 in Core-2+ and the namespace is not reliable as a path. Cannot remove until 3.0 though

@Kaik

This comment has been minimized.

Copy link
Contributor

Kaik commented Dec 14, 2017

What exactly do you expect from it? /home/Kaik/public_html/core15/src/system/?

Well, I needed module root path so whatever is before ModuleName I have used a different method for my purposes. In the meantime, I have noticed that last letter is missing in the output path so I created this issue.
Deprecating it would be very helpful I do avoid deprecated methods so the code I do is more future proof.

@craigh

This comment has been minimized.

Copy link
Member

craigh commented Dec 14, 2017

why do you need module root path? Frankly, you shouldn't be using it.

@Kaik

This comment has been minimized.

Copy link
Contributor

Kaik commented Dec 14, 2017

@craigh

This comment has been minimized.

Copy link
Member

craigh commented Dec 14, 2017

obviously a file path wouldn't work in that situation anyway.

I would recommend you use tagged services for that purpose - not the old method.

@Kaik

This comment has been minimized.

Copy link
Contributor

Kaik commented Dec 14, 2017

Good I will go for tagged services when I will be back working on Dizkus.

@Guite Guite modified the milestones: 1.5.4, 1.5.5 Dec 15, 2017

@craigh

This comment has been minimized.

Copy link
Member

craigh commented Jan 1, 2018

OK I did some experimenting with this. In Zikula 1.4.6 the result is the same as the OP (missing last letter). Even back in 1.4.6, Users was a psr-4 module. So I searched far and wide for a psr-0 module and only found an old release of Dizkus. Indeed, the method returned an appropriate value for Dizkus. I also tried it with a 1.3-era module - PostCalendar and the method failed completely because old style modules are not registered with the kernel.

So, my solution is to make some notes in the 1.5 release cycle about the issue and then immediately remove it from the 2.0 release since psr-0 modules are not supported there. technically, this is a BC break, but I don't see it as a problem because it would never work anyway and the ->getPath() method is what should be used.

@Guite

This comment has been minimized.

Copy link
Member

Guite commented Jan 1, 2018

👍

craigh added a commit that referenced this issue Jan 1, 2018

@craigh craigh closed this Jan 1, 2018

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