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
Conversation
split up all the files and added namespaces. everything broken
This replaces stuff in the Configuration class that hasn't been removed, yet.
This comment has been minimized.
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
improve update() comments
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
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There seem to be 4 plugins that define their own Settings class for configuration by inheriting from the (no longer existing)
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 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. |
lib/plugins/config/core/Loader.php
Outdated
*/ | ||
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); |
There was a problem hiding this comment.
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
This comment has been minimized.
This comment has been minimized.
For plugin 4, settingstree: it's not maintained. You should go ahead and break compatibility. |
This reestablishes the mechanism of adding errors as Sepcial classes to the undefined list.
There are four plugins using the now removed
Ideally these plugins should be updated to use the |
It seems some plugins extend the settings class, like oauth. This then leads to fatal errors:
|
@micgro42 see #2382 (comment) ;-) |
However, there seem to be a few more plugins affected by extending subclasses of |
@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 |
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.
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):
Note: this PR has #2395 already applied.