dokufreaks / plugin-pagelist

Lists DokuWiki pages in a nice looking table or unordered list.
http://dokuwiki.org/plugin:pagelist
GNU General Public License v2.0
30 stars 21 forks source link

Fix deprecated resolve_pageid() #135

Closed dregad closed 1 year ago

dregad commented 1 year ago

Add conditional execution based on DokuWiki version date to maintain backwards compatibility.

Fixes #134

fiwswe commented 1 year ago

Instead of doing a somewhat iffy string compare on the release date, wouldn't it be easier to use the global $updateVersion defined in doku.php? The values seem to be strings that can easily be converted to a float. I think the first release candidate of Igor had the value "52" so something like this might work:

global $updateVersion;
if ((float)$updateVersion >= 52) {
    // This is Igor or later.
} else {
    // This is a version before Igor.
}

Also the .gitignore stuff doesn't really belong in this PR.

dregad commented 1 year ago

wouldn't it be easier to use the global $updateVersion defined in doku.php?

I don't think that would work actually, as users are told to update this value to squelch the upgrade warning check:

If you don't want to upgrade to an announced release you can simply increase the number in doku.php. (note: in older versions, this number was at the top of the file ./conf/msg).

Klap-in commented 1 year ago

Normally these version number checks are not used, for this kind of backward compatibility code. Not sure why, maybe because it not consistent enough with the development version? The typical solution used is checking if the actual function already exists, if not then fall back.

fiwswe commented 1 year ago

Fair enough. I hadn't seen that recommendation.

So ideally there would be a DokuWiki API version number that gets incremented whenever the API changes. Or some more complicated method where each deprecated, removed or modified function/method can be queried to show its status. At the very least it would be good to solve this problem in DW itself, instead of in plugins. Though reading between the lines iI got the impression that DW does not want code to vary depending on version.

However we don't have any of those AFAICT. So after some testing that this sort of string compare actually works correctly in all weird cases I could think of, I'm ok with https://github.com/dokufreaks/plugin-pagelist/pull/135/commits/6cb8817714e41908069ce277f0300a3289d4a81e as is (not that it's my call to make ;-).

Klap-in commented 1 year ago

I see you commit also the .gitignore file. Could you please undo this commit? So far I know this ignore file is already set at top level in DokuWiki’s development version code, and not needed at plugin level. Do you need it at plugin level? Maybe you could put the ignore file to the ignore file as well?

dregad commented 1 year ago

Thanks for your feedback and testing @fiwswe.

Normally these version number checks are not used, for this kind of backward compatibility code. Not sure why, maybe because it not consistent enough with the development version?

Sorry, I'm not a DokuWiki development expert, using version number seemed like a sensible thing to do.

The typical solution used is checking if the actual function already exists, if not then fall back.

OK, I'll try with class_exists() to see if it works, and update the PR if so.

dregad commented 1 year ago

@Klap-in With regards to the .gitignore file

So far I know this ignore file is already set at top level in DokuWiki’s development version code, and not needed at plugin level.

Consider that the plugin's code is managed in its own Git repository, separate from the main DokuWiki repo. This means that any IDE/temp files created during development (e.g. PHPStorm's .idea/ directory) will be marked as Untracked files.

Maybe my development practice is not the same as yours and possibly not the best or recommended DokuWiki way of doing things. This is what I have.

~/dev
  /pagelist      <- clone of dokufreaks/pagelist
  /dokuwiki      <- clone of splitbrain/dokuwiki
    /lib/plugins
      /pagelist  <- symlink to ~/dev/pagelist

I work on the plugin's code from ~/dev/pagelist, so from this perspective the .gitignore file is needed at plugin level. Please advise if I should do things differently.

Maybe you could put the ignore file to the ignore file as well?

This is confusing. What do you mean ?

fiwswe commented 1 year ago

@dregad Put a line containing .gitignore into your .gitignore file. I haven't tried this myself but it sounds reasonable.

Klap-in commented 1 year ago

I clone the plugins directly in lib/plugins/ folder, and after cloning I just rename the folder to the right name. Then you have one project (at level of DokuWiki) that has a idea project, and the extra cloned plugins blend nicely into that. Only setting needed per plugin is that in the idea/phpstorm project settings you have to explicitly add the git repository of the plugin to the list of git repositories used in the project.

I do not need further the .gitignore with this way of working.

dregad commented 1 year ago

Thanks for merging and taking care of the class_exists() fix for me.

I take note of your suggested approach for handling plugins directly within DokuWiki tree, and will try it out to see how that works for me.

For the record, I must say that I really don't understand the strong resistance against having a .gitignore file in the plugin. Even if it's somewhat redundant in the specific use case of cloning the repo under lib/plugins when using PHPStorm,

Klap-in commented 1 year ago

It is not a strong resistance, but I would like to include this PR and I do not see yet if these ignore files were really needed. So far inclusion was not proposed, and such items need maintenance as well.

So far my understanding is that developing these plugins out of the development tree is less common. But might be that this a wrong assumption of me.

Maybe it is useful to extend (and improve as well) this page with a section about how plugins handy can be cloned in the development tree. https://www.dokuwiki.org/devel:git My understanding is that setting up unit tests locally needs also plugins in the tree of the development version of DokuWiki.

fiwswe commented 1 year ago

@dregad

For the record, I must say that I really don't understand the strong resistance against having a .gitignore file in the plugin.

My issue with this is that people may have different preferences as to what should be ignored. Having this in the repository would force everyone to use the same settings, potentially causing a commit storm when the settings are modified to fit personal preferences by multiple committers. But maybe this is not such a big deal in practice because eventually a version that is a superset of all personal wishes will evolve? Take the .DS_Store line for example. Someone not using macOS would not need this line (though it doesn't really hurt either).

Also adding a .gitignore in a PR that addresses something completely unrelated to this file seems wrong. That is the main reason I mentioned this in my feedback.

dregad commented 1 year ago

@fiwswe

people may have different preferences as to what should be ignored

I hear that, but in this specific case I don't believe anyone would want to commit editor-specific and temporary files. Moreover, the exclusions I added to .gitignore are identical to DokuWiki's so it's consistent within the ecosystem.

the .DS_Store line for example. Someone not using macOS would not need this line (though it doesn't really hurt either).

Exactly. It does not make any difference to anyone

Also adding a .gitignore in a PR that addresses something completely unrelated to this file seems wrong

As far as I'm concerned, that is actually the only receivable argument. I included it in the PR as a pure act of laziness 😉

@Klap-in

developing these plugins out of the development tree is less common

Possibly, but regardless of that fact, a Git repository is a standalone unit. JetBrains IDE's inheriting .gitignore settings from a parent directory outside of the git root is not a standard thing, and not having the .gitignore file inside the plugin's repo is causing the not-excluded files to be flagged by Git for anyone not using the IDE for commit operations, making their life more difficult for no reason, and introducing the risk of accidentally committing temp files.

For example, I personally use the Git command-line and SmartGit a lot, so can I never have a clean working tree because of this (untracked files are always present):

image image
Klap-in commented 1 year ago

e.g. in the dokufreak I did found one ignore file set (for ~30 plugins)

Further I guess I don’t understand you setup yet.

Does this help you, solves it for all the plugins at once? https://sebastiandedeyne.com/setting-up-a-global-gitignore-file/