Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PSR-2 refactoring for config plugin #2382

Merged
merged 46 commits into from Jun 15, 2018
Merged

PSR-2 refactoring for config plugin #2382

merged 46 commits into from Jun 15, 2018

Conversation

splitbrain
Copy link
Collaborator

@splitbrain splitbrain commented May 11, 2018

This refactors the config class into several files, cleans up some logic and adds some first tests. It should now be good to merge back into the psr2 branch.

More refactoring in the future should look at (we should make an issue from that):

  • the admin class itself is still a bit ugly and could use some clean up
  • the Settings::html() method needs rework
    • don't return an array of two values - split into two values
    • handle which value to display outside the method (remove the weirdly named $echo var)
    • find a better way to pass translations
  • the mix of errors (like no default) and truly undefined settings isn't very clean
    • ideally problems should be displayed with the actual setting, further up
  • more tests
  • provide a helper plugin that other plugins can use to display a limited config or to change config settings (eg. let an admin plugin change its own config settings)
  • clean up overcomplex methods (see scrutinizer analysis)

Note: this PR has #2395 already applied.

split up all the files and added namespaces. everything broken
@Chris--S

This comment has been minimized.

stripped out all the session stuff
* psr2:
  fix appveyor, download from https always
  Issue #1250, associated unit test
  translation update
  translation update
  fix(TASK_RECENTCHANGES_TRIM event): Add isMedia flag
  rename task event
  add missing visibility indicator and docblocks
  translation update
  refactor: Rename lib/exe/indexer.php to taskrunner.php 🔨
  refactor: Move the remainder of code from lib/exe/indexer.php to TaskRunner::run 🔨
  Fix for Issue#1250 Footnotes break metadata abstract saving
  feat: Trigger new event when changelog is trimmed ✨
  style(TaskRunner): automatic whitespace fixes 🎨
  refactor: Extract code from lib/exe/indexer into new TaskRunner class 🔨
  corectly disable sniff for one file
  translation update
We used to use ptln to ensure the produced HTML source code is somewhat
readable, but with modern HTML inspectors this is no longer necessary
and makes the PHP source code ugly.
Our test suite did not reset the config directory for each test class as
it does for the data directory. In addition it copied all files from the
main config directory over. Both may create an unpredictable state for
tests.

This streamlines the initialization.
* testinit:
  reset config directory for every test
This makes it possible to give test files proper PSR-2 names where the
file is named after the class name.
Instead of having the out() method return empty strings, a new method
tells the writer if a setting should be saved or not. Only then the
out() method is called.
There isn't really unset variables, but we do expect null to be passed
for some of them.

This also moves the update method further up as it's logically the next
step.
* psr2:
  fix: throw RemoteAccessDeniedException if not admin
  feat(RemoteAPI): Add call to delete Users to the remote API ✨
  Make lexer/state stack more understandable - rename lexer $mode property to avoid two different uses of "mode"   variables in the lexer - clarify/improve comments
  removed obsolete language files
  translation update
  decrease php versions by one
  appveyor: updated PHP versions
  another try at fixing appveyor php downloads using curl
In 55a4f13 a bug was introduced that

* forced resaving the config even if it hadn't changed
* prevented setting the value to an empty string once set
@splitbrain

This comment has been minimized.

@splitbrain

This comment has been minimized.

@splitbrain
Copy link
Collaborator Author

splitbrain commented May 25, 2018

There seem to be 4 plugins that define their own Settings class for configuration by inheriting from the (no longer existing) \setting class:

  1. https://codesearch.dokuwiki.org/xref/plugin/sfauth/conf/metadata.php#3 by @cosmocode
  2. https://codesearch.dokuwiki.org/xref/plugin/oauth/conf/metadata.php#8 by @cosmocode
  3. https://codesearch.dokuwiki.org/xref/plugin/txtconf/txtconfig.class.php#7 (not on github)
  4. https://codesearch.dokuwiki.org/xref/plugin/settingstree/settings/settingswrapper.class.php#48 by @fajan

1 and 2 are not a big problem, since I have commit access. Fixing would require creating a new namespaced class in the plugin's namespace. metadata.php would then get the full namespaced class name and Configuration::determineClassName() should pick it up automatically. I think this could even be made backward compatible, the old code simply instantiates the class as given - namespaces included. Would probably be good to implement this in the oauth plugin as an example.

3 and 4 are not straight forward extensions. There seems to be a lot more going on and those plugin will probably break without an update.

*/
public function __construct(ConfigParser $parser) {
global $conf;
$this->parser = $parser;
$this->plugins = plugin_list();
$this->template = $conf['template'];
// allow plugins to remove configurable plugins
trigger_event('PLUGIN_CONFIG_PLUGINLIST', $list);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like that $list is not defined already or used further in this function

@Chris--S

This comment has been minimized.

@fajan
Copy link

fajan commented May 25, 2018

For plugin 4, settingstree: it's not maintained. You should go ahead and break compatibility.

@splitbrain
Copy link
Collaborator Author

There are four plugins using the now removed _cautionList parameter instead of the much simpler _caution one:

  1. https://codesearch.dokuwiki.org/xref/plugin/authhttp/conf/metadata.php#8 by @pief
  2. https://codesearch.dokuwiki.org/xref/plugin/authhiorgserver/conf/metadata.php#9 by @hiorgserver
  3. https://codesearch.dokuwiki.org/xref/plugin/authloginapi/conf/metadata.php#3 by @xelio-plus
  4. https://codesearch.dokuwiki.org/xref/plugin/authsplit/conf/metadata.php#19 by @pief

Ideally these plugins should be updated to use the _caution parameter instead (it's supported since a few years). When the plugins are not upgraded, the warning icons will not be displayed but the plugins and their settings will continue to work as before.

@splitbrain splitbrain changed the title WIP: PSR-2 refactoring for config plugin PSR-2 refactoring for config plugin Jun 1, 2018
@splitbrain
Copy link
Collaborator Author

@Chris--S @Klap-in @micgro42 could you have another look at this? Maybe also try editing your wiki configs with this branch to see if anything breaks.

@micgro42
Copy link
Collaborator

micgro42 commented Jun 1, 2018

It seems some plugins extend the settings class, like oauth. This then leads to fatal errors:

PHP Fatal error: Class 'setting' not found in /home/michael/workspace/dokuwiki/lib/plugins/oauth/conf/metadata.php on line 8

@splitbrain
Copy link
Collaborator Author

@micgro42 see #2382 (comment) ;-)

@micgro42
Copy link
Collaborator

micgro42 commented Jun 1, 2018

However, there seem to be a few more plugins affected by extending subclasses of setting:
https://codesearch.dokuwiki.org/search?project=plugin&project=template&q=extends&defs=&refs=&path=conf%2F&hist=&type=

@splitbrain
Copy link
Collaborator Author

@micgro42 oh, you're right. I missed sfauth (@cosmocode), source (@Chris--S) and src (not on github).

I though about adding some dummy classes inheriting from the correct ones in /inc/deprecated.php however I also adjusted the signature of the html() method and that throws warnings about Signature mismatches... but maybe that is still better than a full fatal error?

This will still throw a signature mismatch warning on PHP7 but at least
it is no longer fatal.
The class gets instantiated for showing the admin menu. There's no need
to always load the whole configuration there. It's only needed when the
Config screen is actually shown. So loading it in handler() instead
should be good enough.
@splitbrain splitbrain merged commit c68e269 into psr2 Jun 15, 2018
@splitbrain splitbrain deleted the psr2-config branch June 15, 2018 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants