[FEATURE] Drop '/index' from generated URLs

[infostreams]

The default URLs that are generated for a page include '/index' at the end if the underlying file is called 'index.md'. In my opinion, this is unnecessary and ugly. I'd rather have a link called '/projects' than a link called '/projects/index'. Both would result in the same page anyway, so it's safe to drop them here for cosmetic reasons.

[Schlaefer]

I'm OK with the idea, the 'index' in the url is not very attractive. But maybe the whole URL generation should be moved from the class constructor to getUrl()?

Fleshing out the currently failing testcase:

    public function testPageHasUrl() {
        // root index
        $this->assertEquals(
            '',
            $this->pageRepository->findByPath('/')->getUrl()
        );

        // root page
        $this->assertEquals(
            'setup',
            $this->pageRepository->findByPath('/setup')->getUrl()
        );

        // sub index
        $this->assertEquals(
            'sub/',
            $this->pageRepository->findByPath('/sub/index')->getUrl()
        );

        // sub page
        $this->assertEquals(
            'sub/page',
            $this->pageRepository->findByPath('/sub/page')->getUrl()
        );
    }

[james2doyle]

The problem about dropping it at this point is that some legacy sites might rely on having the index in there. So maybe making it optional piece of the URL would be better for backwards-compatibility.

[NeoBlack]

please take care of the failing unit test

[NeoBlack]

maybe can this conflict with #160 ?

[infostreams]

I think it makes sense to move the URL generation code into its method. I do think that I would prefer to enable this behavior by default (since I think it's a net 'win'), but because it might break older sites I think we should have an option to disable it with a flag in the config somewhere. Does that make sense? Please confirm before I spend any time on it.

I will then also take care of the failing test.

[james2doyle]

Flag in the config works for me!
On Fri, Dec 19, 2014 at 8:36 AM Edward Akerboom notifications@github.com
wrote:

I think it makes sense to move the URL generation code into its method. I
do think that I would prefer to enable this behavior by default (since I
think it's a net 'win'), but because it might break older sites I think we
should have an option to disable it with a flag in the config somewhere.
Does that make sense? Please confirm before I spend any time on it.

I will then also take care of the failing test.

—
Reply to this email directly or view it on GitHub
#170 (comment).

[NeoBlack]

Dislike the configuration flag.
I would prefer the following:

1. generate URL without index
2. take care of URLs with index and redirect to URL without index

move the logic into a method is ok for me,

[infostreams]

Well I'm open for anything but I'm not sure if I understand what you mean with the first part of 2). Do you mean that URLs like

http://example.com/somewhere/index

should redirect to

http://example.com/somewhere ?

and that by default, the /somewhere/index url is not even generated?

[NeoBlack]

yes, this was the idea :)

[james2doyle]

Ok that is a little nicer

On Fri Dec 19 2014 at 8:51:08 AM Frank Nägler notifications@github.com
wrote:

yes, this was the idea :)

—
Reply to this email directly or view it on GitHub
#170 (comment).

[Schlaefer]

maybe can this conflict with #160 ?

Shouldn't be affected.

[infostreams]

Ok, I will do that. Not today unfortunately :-( In the mean time, check out this nice plugin https://github.com/infostreams/snippets :-)

[infostreams]

Ok, all done. I had to adjust the .htaccess as well for everything to work - it would redirect valid requests such as http://localhost/tmp/Phile/sub/index to the non-existing page http://localhost/sub instead of to the intended http://localhost/tmp/Phile/sub. Also, the test is fixed, the URL generation is in it's own method, and it strips '/index' more carefully now.

[Schlaefer]

I had to adjust the .htaccess as well for everything to work

Now we are talking #159/#160 business. ;) If this is merged they should be closed.

PS: Is it possible to handle pull requests in a more timely fashion, so that we don't fall over each other (esp. if there's no disagreement)? I'm not talking 24 hours, but maybe with ca. 3 months as an upper limit? 😘

[Schlaefer]

My original thought was that all code of this method could go into getUrl() and URL-generation isn't triggered in the constructor. Anyway, at least the method should be protected to allow easy refactoring later, getUrl() already offers public access.

[NeoBlack]

agree, buildUrl must be protected, public access should only be possible with getUrl()

[james2doyle]

I'm good with this change. I do think the additional buildUrl function might not be necessary. It might be nice to also add an event for when the url has been generated?

I can see it being useful for certain plugins.

Anyway, I like this and if @NeoBlack is good, I can merge this manually since there is a merge conflict.

[NeoBlack]

-1 as @Schlaefer said, buildUrl() must be protected.

[infostreams]

Ok, I made buildUrl() protected, and I no longer invoke it from the constructor. However, I don't think we should include these things in the getUrl() method. I have a number of arguments for that, one of which is performance related, and the other two are philosophical.

The first philosophical one is that a getter is typically only used to report on an object's internal state. Changing the internal state and then reporting it might be common, but it's not something that I prefer.

The second philosophical one is that this would mean that you can set the filePath of the page (with the public setFilePath() method), after which you would have an inconsistent internal state until getUrl() is called. Any code doing a quick $this->url would break. The external state would remain consistent, however, because you can only access the url via the getUrl() method, but via the principle of least astonishment I think this would be a bad thing anyway.

Third, if we put this stuff in the getUrl() method, it would be run every time getUrl() is run - and it would come to the same answer every time. That's unnecessary and inefficient, and in light of the above arguments I've decided to call this functionality in the setFilePath() instead. That would solve all three objections at the same time, in a clear and simple way. Hope you agree ;-)

[Schlaefer]

My thought when I glanced over it yesterday was that it's bag of hurt that needs extensive refactoring, because path (Model) and URL (Router) are intermingled and used quite synonymously.

The second philosophical one is that this would mean that you can set the filePath of the page (with the public setFilePath() method), after which you would have an inconsistent internal state until getUrl() is called.

$folder passed into the constructor is not the folder where the page lies in, but per default the 'content' folder path of the whole installation which should be in sync with the URL-scheme to resolve the request. It can't get out of sync because otherwise the page isn't found.

Also the events in the constructor could change the path which then affect the URL. All of this is not happening anymore.

Did I mention the bag-o-hurt? ;)

Third, if we put this stuff in the getUrl() method, it would be run every time getUrl() is run - and it would come to the same answer every time.

I thought about it, but I would roll with it. Worst case: twice per page per request when uncached having a top and bottom menu? But your choice is OK with me too. :)

---

With the current changes the $folder argument in the constructor is unused and should be removed. $folder in buildUrl() is "just" a global constant. But this change would also break backward-compatibility.

To keep BC, keep further changes to a minimum and get the pull request done I would still inject $folder into the constructor (maybe rename it) and store it as protected class property which can be accessed by buildUrl()? Ignore setFilePath() when resolving getUrl() as it was ignored before. Leave it for another ticket/pull request.

[infostreams]

Ok, I've added a protected property $contentFolder, and left the rest as-is. The URL is still re-built on setFilePath as the URL actually depends on $filePath and I don't want to introduce code that makes the internal state of the object unpredictable.

I agree that there's quite a bit of weird stuff going on in this class though. I haven't found a use for the getFolder() method yet, for example, and I think it's weird that there's a getUrl() but not a setUrl() (even though I understand the reasons). Also, the getPreviousPage() and getNextPage() methods seem out of place. But perhaps a more thorough refactoring needs to wait...

Anyhow, let me know if you can merge this now.

[infostreams]

So can anyone either merge this now or tell me what needs to be changed so it can be merged? It's been a while...

[james2doyle]

Im good with the change

Ping @NeoBlack

[infostreams]

Ok, this is ridiculous. Why do I even bother contributing code if you're not serious about accepting it? Do you want help, do you want contributions, or do you just want to discourage people? Because that's what you're doing. At this point I'm:

1. Disappointed that I find (frankly) relatively trivial and superficial bugs like this one: urldecode URIs before turning into filename #175
2. Disappointed in the lack of enthusiasm about outside code contributions
3. Disappointed in the glacier speed of development
4. Disappointed that I didn't choose something like 'Grav' instead (even though it's far too heavy for my taste).

You have something good here. But you can't develop it into something proper without outside help and a sense of community. Yet here I am, able & ready to contribute, but you ended up killing my enthusiasm. You can say that's not true and it's not fair and I was very busy and whatever, but the fact is that I could have contributed significant amounts of good code to this project but that I'll now think twice before I try to get anything through to you guys. This was a relatively small and insignificant change, but at this point it has taken more than 3 weeks to get it accepted. You think that's encouraging? No, it's off-putting.

If you want my advice: change how you accept code from outside people. You seem to all have a veto on pull request. Drop it. Change the rules. If one admin thinks it's good, then accept it. If other people disagree, revert or change. Why have admins if you can't trust their judgement? Or make a rule that pull requests are either denied or accepted within 2 or 3 days. The way you're organized now makes you SLOW and unresponsive, and it's severely off-putting for outside people.

Do whatever you want with this pull request, or with my other ones. I'll probably still submit a pull request if I find a bug that prevents me from achieving what I want to achieve, but you've effectively killed my enthusiasm for contributing code to this project.

[NeoBlack]

+1 ok for me

[james2doyle]

Thanks for your contribution.

Read this next sentence very carefully: the reason that things are slow, is also the reason that things are good.

This was not a merge instantly request. There were concerns about backwards compatibility, there was a failing unit test, and concerns about where the code should be placed. There was 9 commits since the PR was posted, that is not trivial to me.

glacier speed of development

If you think a couple of weeks is a long time, then you're in for some real disappointment. I waited over 1 year to have an issue addressed on Anchor CMS. People work on these projects in their spare time. If they are going to be chastised and harassed for providing FREE tools and frameworks, they are not going to want to give up their unpaid time to work on these projects anymore. We are all on different timezones, with separate lives, we can't all align perfectly each time.

Disappointed that I didn't choose something like 'Grav' instead

Grav is funded by rockettheme. If you want to toss a few thousand dollars at the contributors of this project, that would be fantastic. I have already put money in this project, because I have been working on Phile on my company dime. I also had one of my other employees work on plugins. @NeoBlack has also found some of his companies time to work on Phile as well.

If other people disagree, revert or change

You clearly haven't been involved in any large, public, projects. How would you feel if someone made a breaking change to a library/tool you use, you pulled the latest changes, and it busted your project? To me, thats a big fuck you to anyone using something I made.

I am happy that you are helping, and appreciate your code and fresh input, it's great. If you want to cultivate an attitude like this on a place like GitHub (or anywhere else on open source projects), you are going to be treated a lot less politely than I have treated you here.

[infostreams]

Three points:

1. I understand that it's better to take your time to do things, and that good things take time - but in this case there was 3 or 4 days of activity, followed by 17 days of inactivity. Those 17 days did nothing to improve software quality. Additionally, this project has seen almost no commits since last August, which increased my fear that nothing would happen for another few months.

   I am completely on board with your point regarding not breaking the software, though. But isn't it better to keep development and release branches separate anyway? Like this famous article describes? That way you can just accept decent looking pull requests in your development branch, or in a feature branch, and then proceed from there. And then only when everything is ready you merge it back into master, so that the "average user" never even sees the broken code.

2. I agree that 3 weeks is not necessarily a long time to wait. Three weeks of silence sucks though, especially if we're (actually: I'm) just waiting for someone to press 'merge' or 'reject'.

3. Thanks for Phile and all the work you put in, and sorry for being grumpy and phrasing it the way I did.

[james2doyle]

Yeah it's fair. Doing a bunch of free work and then having silence isn't a great motivator.

In context, this PR did cross over the Christmas break and New Years. I had a 2 week vacation where I didn't have much activity on any projects.

I know there is little activity on Phile, which can be seen as good and bad. On the good side, it's pretty stable. On the bad side, maybe patches aren't being applied regularly.

[Schlaefer]

my 2 cents nobody asked for: what really needs to be done

• pull requests should be addressed in a more timely fashion
• a better release/branch management

Provide a ready to use release package and don't link breaking dev-snapshot on the homepage (#158). People run into this all the time (#163 #165 #167 #173). If pull request take some time, mention it somewhere, maybe a contribote.md is good idea? Using a separate develop branch which is not immediately linked on the homepage you can also except changes more quickly and test/revert unforeseen breakage before the next official release. No harm done.

If you want the project to thrive and contributions addressing those points is more important than any other features or core code improvements. Imho. Yeah it's boring, but it needs to be done. If you just want to put the code out there because it might be useful to others – which is fine too – it's probably a good idea to communicate that. As I said above, I totally get that people have other obligations and a real life, but contributors just expect faster response times on an actively maintained project nowadays.