Collected fixes and improvements #85
No reviewers
Labels
No labels
Component: User interface
Component: Wymeditor
Help wanted
Level: Difficult
Level: Easy
Level: Moderate
Pagetype: Festival
Pagetype: Mailinglist
Pagetype: Peer reviewed article
Pagetype: Text
Privacy GDPR AVG
status: has conflicts
Status: Needs changes
Status: Needs discussion
Status: Needs review
Status: Ready to merge
Status: Waiting for response
Type: Bug
Type: Enhancement
Type: Question
Usecase: De Stadsbron
Usecase: Koppelting
Usecase: MeetjeStad
Value: Coders
Value: Security
Value: Users
Value: Visitors
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
harmen/hypha!85
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "tomerge"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Overall, this PR looks good! I've left some inline comments, which are mostly about pieces of code which I believe would be better placed in a different or separate commit, or about commit messages. In general, commit messages could be a bit more verbose, especially because some of these commits are bigger than would be ideal.
I've not reviewed the Article and WymEditor commits in detail, since I really ought to be doing othe work today :-p
@ -1,7 +1,7 @@.idea/system/wymeditor//data/pages/This commit needs a better commit message too. It does not really add WYMEditor, since we were already using that. It does add the source or WYMEditor, but also makes a lot of other changes to how WYMEditor works. Stating these changes explicitly would be better. Also, please mention the version of WYMEditor that is added, for future reference (version number and/or git hash), and whether any changes were made to the upstream wymeditor code (if so, adding the code verbatim first and then modifying the code in a separate commit would be preferred).
This commit adds third-party sources. It would be good if the commit message lists the upstream locations and versions of these.
@ -73,2 +63,4 @@$hyphaQuery = $hyphaRequest->getRelativeUrlPath();$hyphaUrl = $hyphaRequest->getRootUrl();// Shortcut for direct file requestsThese readdir -> scandir changs seem unrelated to the core of this commit? Not a big deal, though.
@ -84,6 +80,7 @@Interesting approach to autodetect datatype classes. Not sure if I really find it elegant or hacky, though :-p
In any case, this change would be better in a separate commit, or should at least be mentioned by the commit message.
These are whitespace-only changes that might have been better in a separate commit?
Isn't $hyphaUser deprecated too?
@ -0,0 +88,4 @@}return $this->dictionary;}Should this be cached? Now, I think, every call to __ for translation reloads the entire translation file?
This also means that no two users with usernames only differing in case can ever exist. Does user registration use this same function to guarantee this?
Should this have a comment saying it only modifies hypha.xml and you should usually use deletePage instead?
Should we really delete pages? Or mark them as deleted and allow an admin to view a list of deleted pages? Probably a separate discussion that should not block this merge, though.
Is this "is private" check (or rather, "normalize boolean" check) something to put in a util function (which should probably return bool, not 'on'/'off'? Probably something that is fine for a new commit after merging this PR, though.
I'm not really sure what kind of improvements this commit really makes. Ideally, the commit message should explain any change in behaviour, just saying "improved" is vague unless it is really obvious from a very short commit (and even then, explaining is better).
This $pageListDatatype variable is introduced, but not used anywhere. It also seems unrelated to private handling, making me suspect this line belongs in a later commit?
Also, adding this line is the only thing changed by this entire patch hunk I believe (other than whitespace changes), so the entire thing could then be moved.
This seems unrelated to styling?
This seems unrelated to styling?
@ -131,3 +130,3 @@$_name = $hyphaRequest->getPageName();$_name = $request->getPageName();if ($_name === null) {$_name = hypha_getDefaultPage();Doesn't this mean the user gets a 404 when they're not logged in, rather than a message saying they should login? This is better from a strict security perspective, but leakage of private page names does not seem like a big issue here and could be less important than usability?
Is this session language stuff preserved anywhere? If it is no longer relevant and removed, the commit message might state this.
@ -340,3 +361,3 @@notify('success', __('festival-successful-signup-for') . $this->getConfig('festival-title'));$contribute_url = $hyphaUrl . $hyphaLanguage . '/' . $this->pagename . '/contribute/' . $participant->getAttribute('xml:id') . '/' . $participant->getAttribute('key');$contribute_url = $hyphaUrl . $hyphaContentLanguage . '/' . $this->pagename . '/contribute/' . $participant->getAttribute('xml:id') . '/' . $participant->getAttribute('key');In addition to introducing RequestContext, this commit also introduces the split between UI language and content language. This was originally a separate commit, I think. It is probably too much work to split these now (since then the original loadLanguage would need to be re-introduced and modified), but this split should probably be mentioned in the commit message.
@ -97,6 +97,7 @@"changed-css" => " edited the website's CSS layout","changed-html" => " edited the website's HTML structure","changed-page" => " edited the following page: ","deleted-page" => " deleted the following page: ",These changes add strings that are not used until the next commit, so they would make more sense in the next commit?
@ -73,2 +63,4 @@$hyphaQuery = $hyphaRequest->getRelativeUrlPath();$hyphaUrl = $hyphaRequest->getRootUrl();// Shortcut for direct file requestsMoved to own commit
added @deprecated
@ -0,0 +88,4 @@}return $this->dictionary;}Nice catch! Implemented cache
Added extra line in commit message
@ -97,6 +97,7 @@"changed-css" => " edited the website's CSS layout","changed-html" => " edited the website's HTML structure","changed-page" => " edited the following page: ","deleted-page" => " deleted the following page: ",Moved to other commit
From a scope perspective I think it's a bit weird that
hypha_deletePage"knows" aboutdeletePage. I think the readme should contain a chapter containing datatypes and this information should be placed there.Good idea! This can be implemented if needed.
@ -84,6 +80,7 @@I understand that it looks a bit hacky but I do believe this to be less error prone.
I've put it in a separate commit.
Moved $pageListDatatype to other commit
Agreed, a util function would be better.
I've updated the commit message to explain the change.
@ -131,3 +130,3 @@$_name = $hyphaRequest->getPageName();$_name = $request->getPageName();if ($_name === null) {$_name = hypha_getDefaultPage();When, when a not-logged-in user tries the access a private page they will see a login-to-view message.
Yes, no two users with usernames only differing in case can ever exist.
Yes, the user registration uses this same function to guarantee this.
The
$filecan contain non-strict html that would cause errors, therefore those errors are ignored. In my opinion the non-strictness is only for styling purposes.Changed commit message to "improved UX".
This actually ignores third-party sources.
@ -1,7 +1,7 @@.idea/system/wymeditor//data/pages/The commit is split as suggested, the commit message is updated.
Agreed. I'll do that next time.
Did you push your changes? I can't see anything in the commit message here: https://github.com/PlanBCode/hypha/pull/85/commits/837b9eb27d98d71c2d745fdc9b09497c81e78f8e
I think I reverted the commit when something when wrong and I forgot to put back the commit message. Now again I've added an extra detail in the commit message.
I created #88 for this.
@ -131,3 +130,3 @@$_name = $hyphaRequest->getPageName();$_name = $request->getPageName();if ($_name === null) {$_name = hypha_getDefaultPage();I've added a bit more to the commit message for this change:
I've moved the libxml change to its own commit, and clarified in the code that this only ignores unknown tag errors, not all errors as I previously thought when quickly reading the code.
I meant the commit this change is in, not this particular change (I just took the first change in the commit, since you can't comment on a commit anymore in github). However, I see now that this is not really clear.
In any case, I meant files like normalize.css. I've now found the source for this file and put it in the commit message. I've also removed chota.css and cclogo.png, both of which seemed unused (and chota.css was in data/ rather than data/css, which seemed odd).
For the dosis font, I couldn't find a canonical source. Perhaps we should clarify the source along with the license (see #89).
I've moved this check into index.php, so it can incorporate the root url. This fixes an infinite redirect when trying to access e.g. /en/SomePage without a datafile, which would redirect to /en/hypha.php, which would keep redirecting to itself.
I've made some more small changes (as commented) and force pushed to this branch. As far as I'm concerned, this PR is ready for merging. I'll leave it to you guys tonight to do a final review and do the actual merge.
Wie durft er op de knop te drukken?
Ik! Gemerged :-D
Top. Great step for mankind!