Skip to content

Fix namespace detection for pages in root#30

Merged
glensc merged 9 commits into
glensc:mainfrom
Miro-Collas:1.-Fix-for-pages-in-root
Mar 1, 2024
Merged

Fix namespace detection for pages in root#30
glensc merged 9 commits into
glensc:mainfrom
Miro-Collas:1.-Fix-for-pages-in-root

Conversation

@Miro-Collas

@Miro-Collas Miro-Collas commented Sep 9, 2023

Copy link
Copy Markdown

Add support for changed pages in the root namespace.

Currently the code works very well except when changes are made to pages in the root namespace. Say you have a page :test. The function isValidNamespace is receiving 'test' in the $namespace function argument, rather than ':' or an empty string.

isValidNamespace seems to be called from processEvent(PageSaveEvent $event) in action.php, so it looks like the namespace set in the $event argument is incorrect for pages in the root.

Fixes #25

Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread event/PageSaveEvent.php Outdated
Comment thread helper/Config.php Outdated
@glensc

glensc commented Jan 23, 2024

Copy link
Copy Markdown
Owner

describe what the mr does (title/body).

@Miro-Collas

Copy link
Copy Markdown
Author

describe what the mr does (title/body).

Sorry, I don't know what you're referring to

@Miro-Collas Miro-Collas changed the title Fix for pages in root Fix for pages in root; ref issue #25 Jan 24, 2024
@glensc

glensc commented Jan 25, 2024

Copy link
Copy Markdown
Owner

describe what the mr does (title/body).

Sorry, I don't know what you're referring to

sorry, gitlab addict. "mr" was meant as "pr" for pull request.

pull request should be standalone, i.e in title and body describe what the change is. without needing to click additionally issues with long threads and trying to figure out what part of the issue is actually maybe implemented here.

@Miro-Collas

Copy link
Copy Markdown
Author

describe what the mr does (title/body).

Sorry, I don't know what you're referring to

sorry, gitlab addict. "mr" was meant as "pr" for pull request.

pull request should be standalone, i.e in title and body describe what the change is. without needing to click additionally issues with long threads and trying to figure out what part of the issue is actually maybe implemented here.

Done

@Miro-Collas

Copy link
Copy Markdown
Author

I have to say, I have submitted PRs for multiple other plugins, to fixes errors and warnings, and I have never had them nitpicked to death like this. If you're not happy with the PR, then I'll just close it and delete my fork.

@glensc

glensc commented Jan 25, 2024

Copy link
Copy Markdown
Owner

I have to say, I have submitted PRs for multiple other plugins, to fixes errors and warnings, and I have never had them nitpicked to death like this. If you're not happy with the PR, then I'll just close it and delete my fork.

That's a pity that you have not seen a repository where they do code reviews. I definitely agree most of the plugins written are of very pool quality.

@glensc glensc changed the title Fix for pages in root; ref issue #25 Fix for pages in root Jan 25, 2024
@glensc glensc changed the title Fix for pages in root Fix namespace detection for pages in root Jan 25, 2024
@glensc

glensc commented Jan 25, 2024

Copy link
Copy Markdown
Owner

So, after codereview and applying the changes, the change is just a single change now:

@Miro-Collas

Copy link
Copy Markdown
Author

I have to say, I have submitted PRs for multiple other plugins, to fixes errors and warnings, and I have never had them nitpicked to death like this. If you're not happy with the PR, then I'll just close it and delete my fork.

That's a pity that you have not seen a repository where they do code reviews. I definitely agree most of the plugins written are of very pool quality.

So, here's the thing. I am not a PHP programmer. I am definitely not a github guru, I barely know how to use it. I merely manage a dokuwiki installation. When we updated to the most recent dokuwiki, we found logs being spammed with warnings due to deprecated code. Some plugins had not been updated in quite some times, so I decided to try to fix the problems, and then contribute my (suggested) fixes back to the community, as a way of saying thank you. In fact, you have one of my fixes pending here: glensc/dokuwiki-plugin-pageredirect#47

Having done all of that, I decided to try to implement something we needed, which was reporting of edits in the root name space to discord. I did that. Then since the code there is almost the same as the code here, I thought to submit the changes here as well, hoping they would be helpful to anyone who does use this plugin.

As a "hobbyist", your response really makes me reticent to ever make contributions again. I am sure I could have done things better, and it is good to learn - but at the same time, working with PHP and with github is not meant to be part of my skill set. So, perhaps next time someone offers you code, be a bit more forgiving if the submission isn't perfect and up to your standards.

@glensc

glensc commented Jan 26, 2024

Copy link
Copy Markdown
Owner

Can, you add your code review approve if this looks ok to you? otherwise, this has to wait until I test the code myself.

@Miro-Collas

Copy link
Copy Markdown
Author

Can, you add your code review approve if this looks ok to you? otherwise, this has to wait until I test the code myself.

  • I don't know how to do that
  • as noted, I can't test it either

@glensc

glensc commented Jan 31, 2024

Copy link
Copy Markdown
Owner

@Miro-Collas Miro-Collas left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok to me.

@Miro-Collas

Copy link
Copy Markdown
Author

You should have review now link and review changes button on any github repository:

* https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/approving-a-pull-request-with-required-reviews

OK did that - the only option I had available was "comment"; the other two were disabled.

@glensc

glensc commented Jan 31, 2024

Copy link
Copy Markdown
Owner

uh, what other two?

here's what I see some repo I do not have write access:

image

and clicking "review changes" text (or the Files tab as described in github documentation)

image

@Miro-Collas

Copy link
Copy Markdown
Author

If I click "Review changes", I get this:
review_changes
The only available option is Comment

@glensc

glensc commented Mar 1, 2024

Copy link
Copy Markdown
Owner

The "Submit review" button is green. just press it

@glensc glensc merged commit 1b212aa into glensc:main Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support reporting changes in root namespace.

2 participants