harmen/hypha

Fork 0

Let (de)wikify handle anchors in links #235

Merged

matthijskooijman merged 1 commit from fix-anchors into master

2019-11-25 11:23:00 +00:00

matthijskooijman commented

2019-03-20 14:30:08 +00:00

(Migrated from github.com)

Before, it would treat the anchor part of a link as part of the
pagename and would create empty pages called e.g. page_name#anchor.
Since the links would still be dewikified back to their original value,
the anchors would actually work, but once these dummy pages are deleted,
dewikifying of the links breaks.

To simplify parsing the urls in wikify and dewikify, a regex is used
instead of adding another layer of explode calls.

Before, it would treat the anchor part of a link as part of the pagename and would create empty pages called e.g. `page_name#anchor`. Since the links would still be dewikified back to their original value, the anchors would actually work, but once these dummy pages are deleted, dewikifying of the links breaks. To simplify parsing the urls in wikify and dewikify, a regex is used instead of adding another layer of explode calls.

laurensmartina (Migrated from github.com) approved these changes

2019-03-30 10:06:46 +00:00

laurensmartina (Migrated from github.com) left a comment

A unittest would come in handy now, perhaps we should introduce them.

system/core/pages.php

Regular expressions are, for a lot of people, hard to read. A comment, explaining what it does, would be helpful.

tammoterhark (Migrated from github.com) reviewed

2019-03-30 10:13:05 +00:00

system/core/pages.php

I do agree. For non-programmers like me, regex are hell.

matthijskooijman commented

2019-04-25 09:50:07 +00:00

(Migrated from github.com)

I've added some comments, simplified the regex (while explaining it I realized it was overcomplicated) and rebased.

@laurensmartina @tammoterhark, does this comment make things more clear for you?

I've added some comments, simplified the regex (while explaining it I realized it was overcomplicated) and rebased. @laurensmartina @tammoterhark, does this comment make things more clear for you?

laurensmartina (Migrated from github.com) requested changes

2019-04-30 21:48:46 +00:00

laurensmartina (Migrated from github.com) left a comment

Handling the uri can and should be done in the same manner as in HyphaRequest.

Handling the uri can and should be done in the same manner as in `HyphaRequest`.

system/core/pages.php

Seems like duplicate behaviour to HyphaRequest. The method in HyphaRequest is way more extensive and seems to cover more cases.

Seems like duplicate behaviour to `HyphaRequest`. The method in `HyphaRequest` is way more extensive and seems to cover more cases.

matthijskooijman commented

2019-05-02 09:30:14 +00:00

(Migrated from github.com)

Good point, I'll see how we can adapt this to involve HyphaRequest (probably requires some changes to HyphaRequest as well).

matthijskooijman commented

2019-11-20 07:27:27 +00:00

(Migrated from github.com)

@laurensmartina, it turns out using HyphaRequest for parsing these urls is a bit more involved and requires changes to HyphaRequest. How about we merge this as-is with a regex (since it does fix something right now) with a TODO and then fix this as part of #274?

If you agree, I'll do one more pass over the code, add the TODO and merge it :-)

@laurensmartina, it turns out using `HyphaRequest` for parsing these urls is a bit more involved and requires changes to `HyphaRequest`. How about we merge this as-is with a regex (since it does fix something right now) with a TODO and then fix this as part of #274? If you agree, I'll do one more pass over the code, add the TODO and merge it :-)

laurensmartina commented

2019-11-25 09:21:24 +00:00

(Migrated from github.com)

I've added some comments, simplified the regex (while explaining it I realized it was overcomplicated) and rebased.

@laurensmartina @tammoterhark, does this comment make things more clear for you?

Yes, it makes it clear.

> I've added some comments, simplified the regex (while explaining it I realized it was overcomplicated) and rebased. > > @laurensmartina @tammoterhark, does this comment make things more clear for you? Yes, it makes it clear.

laurensmartina commented

2019-11-25 09:23:15 +00:00

(Migrated from github.com)

@laurensmartina, it turns out using HyphaRequest for parsing these urls is a bit more involved and requires changes to HyphaRequest. How about we merge this as-is with a regex (since it does fix something right now) with a TODO and then fix this as part of #274?

If you agree, I'll do one more pass over the code, add the TODO and merge it :-)

agreed

> @laurensmartina, it turns out using `HyphaRequest` for parsing these urls is a bit more involved and requires changes to `HyphaRequest`. How about we merge this as-is with a regex (since it does fix something right now) with a TODO and then fix this as part of #274? > > If you agree, I'll do one more pass over the code, add the TODO and merge it :-) agreed

laurensmartina (Migrated from github.com) approved these changes

2019-11-25 09:23:33 +00:00

No reviewers

laurensmartina

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

harmen/hypha!235

No description provided.

Rows
Columns