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 Adoption #2358

Open
wants to merge 110 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@splitbrain
Owner

splitbrain commented Apr 27, 2018

This is the beginning of the refactoring to the PSR-2 coding standard as announced on the mailing list.

For now I only care about fixing errors that can not be fixed automatically. Any help is appreciated.

The following gets you started. It creates a todo file listing all the problems that need manual fixing. Yes, its a lot.

cd _test
wget https://squizlabs.github.io/PHP_CodeSniffer/phpcs.phar
chmod 755 phpcs.phar
./phpcs.phar --no-colors |grep -vF ' [x] ' |grep -E '^( |FILE:)' >todo && (grep -E '^ +[0-9]' todo |wc -l)

You should run this on a fresh checkout to avoid checking a whole bunch of 3rd party plugins.

Not all of the problems can be fixed. We will need to define a lot of exceptions to keep backward compatibility. We will probably also define exceptions for things we want to fix later.

We might also end up deciding to not fix a lot of files for now and only do them when we refactor them anyway.

PHPCS Advanced Usage explains how to exclude sniffs.

For now I want to fix as much as possible, exclude the rest, finally run the automatic fixing and then merge this back.

People reviewing this PR should probably do so on a commit-by-commit base.

splitbrain added some commits Apr 27, 2018

remove DOKU_INC checks
There is no need for this check, since these files should not have any
main code that is executed on direct call.

Fixes PSR1.Files.SideEffects.FoundWithSymbols
do not check for namespace use, yet
We have to support lots of legacy code without namespaces
disable some sniffs on things we can not change
because backwards compatibility. There will be more excludes. This is
just a start.
line lengths shortened
This makes sure all files use line lenghts shorter than 120 characters.

This is a quick fix. It might not always be the nicest change.

splitbrain added some commits Apr 27, 2018

visibility fixes
First start at declaring visibilites for methods and properties. Still
missing: the parser/renderer stuff and the plugins
plugin prototype adjustments
They are now proper abstract classes
fixed broken tests
I also introduced an auto loaded namespace for the tests.
@@ -48,7 +48,11 @@ public function cssStyleini($tpl, $preview=false) {
if (file_exists($incbase . $basename . '.' . $newExtension)) {
$stylesheets[$mode][$incbase . $basename . '.' . $newExtension] = $webbase;
if ($conf['allowdebug']) {
msg("Stylesheet $file not found, using $basename.$newExtension instead. Please contact developer of \"{$conf['template']}\" template.", 2);
msg(
"Stylesheet $file not found, using $basename.$newExtension instead. '.

This comment has been minimized.

@Klap-in

Klap-in Apr 27, 2018

Collaborator

Single quotes and double quotes are combined.

@@ -838,6 +846,7 @@ function clientIP($single = false) {
*
* @link http://www.brainhandles.com/2007/10/15/detecting-mobile-browsers/#code
*
* @deprecated 2018-04-27 you probably want media queries instead anyway

This comment was marked as resolved.

@Klap-in

This comment was marked as resolved.

@splitbrain

splitbrain added some commits Apr 28, 2018

visibility definition for the renderers
I made a lot of things public that probaly should be protected. But many
syntax plugins do access renderer mechanisms directly, so better stay on
the safe side here.

The base renderer is now abstract.
split out parser modes into their own files
This moves all the parser classes into their own namespace and files.
Next up are the handler classes.

I'm not sure about the namespace, yet. A nested namepspace Parser\Modes
would probably make more sense... we'll see.

This also removes the duplicated coded in the Plugin mode. We now use
the plugin trait and can inherit from AbstractMode instead.
fixed wrong quoting
Used the wrong quotes to split up the string. But actually, we can just
use a linebreak. Doesn't matter for HTML.
@micgro42

This comment was marked as off-topic.

Collaborator

micgro42 commented Apr 30, 2018

This PR (or this refactoring effort in general) might also be a good place to remove the suppression of PHP E_NOTICE errors. Modern PHP throws notices mostly in sensible places and if we really think a notice is an unavoidable false positive at a given line, then we can still @-suppress it individually. (But this should rarely be the case)

What do you think?

@splitbrain

This comment was marked as off-topic.

Owner

splitbrain commented Apr 30, 2018

Let's tackle one thing at a time.

splitbrain added some commits Apr 30, 2018

split handler.php into multiple files
For now I left Doku_Handler itself as it were. We will need to keep the
class name around for backwards compatibility but should move the class
itself.

I introduced a new ReWriter Interface to formalize how the various call
writer implementations are accessed.

There are a whole bunch of doc blocks missing.
Merge branch 'master' into psr2
* master:
  typo
  link to avanced geshi options. fixes #2352
  unlock in cancel action
  unlock pages on viewing them
  add user interface back to resendpwd action. fixes #2349
@splitbrain

This comment has been minimized.

Owner

splitbrain commented Apr 30, 2018

I started to split up inc/handler.php and there are a few things I need help/feedback with.

I introduced a new ReWriterInterface to formalize how the different CallWriters (like Lists or Footnotes) are accessed by the handler. Now process() returns the original call writer and is a required method.

I wonder about some things:

  • is ReWriter the proper term? Those classes seem not so much rewrite existing calls than rather process new ones for a certain time?
  • some doc blocks are still missing. Especially the interfaces should be documented much better. @Chris--S could you help with that?
  • the Block class is not implementing the ReWriterInterface, but actually seems to be the only proper rewriter in the literal sense of the term. It also has a process() method but works different from the other classes. I wonder if it should be mixed in the same namespace as the others.
  • Doku_Handler_Parse_Media() is a function, not a class (despite the upercase name). It does not fit with anything and I wonder what to do with it. Where does it belong to?

For now I haven't touched Doku_Handler itself. I was wondering if we should use this opportunity and restructure our whole parser/handler stuff for the default syntax to be a little bit closer to what we do for syntax plugins. Currently we have some part of one syntax in the ParserMode classes and some other part as a function in Doku_Handler. I wonder if we should combine both, so the parsing and handling is both done in the same class and only rendering is left to the renderer... In the end we could probably implement all our syntax as plugins... But I guess this taking this PR a bit too far for now.

@Chris--S

This comment has been minimized.

Collaborator

Chris--S commented Apr 30, 2018

I'll take a look.

@LarsGit223

This comment has been minimized.

Collaborator

LarsGit223 commented Apr 30, 2018

For now I haven't touched Doku_Handler itself. I was wondering if we should use this opportunity and restructure our whole parser/handler stuff for the default syntax to be a little bit closer to what we do for syntax plugins. Currently we have some part of one syntax in the ParserMode classes and some other part as a function in Doku_Handler. I wonder if we should combine both, so the parsing and handling is both done in the same class and only rendering is left to the renderer... In the end we could probably implement all our syntax as plugins... But I guess this taking this PR a bit too far for now.

I agree that it would get too far for this PR. But I also agree that it would be good to have some re-design done on all the handler/parser stuff. Some things might be off topic but this is just coming to my mind:

  • I remember in the ODT renderer I did not like to save the headers in the renderer. I couldn't re-use the headers stored by the DokuWiki core because saving was limited to the maxtoclevel setting. IMHO care should be taken to examine to separate data of global interest from renderer specific settings (like maxtoclevel).
  • It would be cool if it would be possible to have more than one XHTML renderer in parallel. I think I read in a thread that it would be problematic to do this by file extension because of all the plugins out there. But maybe it could be done by some special syntax, e.g. ~~MIME:xyz~~
  • A common event system for syntax events. E.g. to be notified of opening/closing of a syntax element before it is inserted in the parsing tree so that the tree can be modified before (e.g. like for creole: automatically close element A if element B is opened)
@michitux

This comment has been minimized.

Collaborator

michitux commented Apr 30, 2018

Before you completely restructure the handler and parser, it would be great if you could have a short look at what the move plugin does in order to ensure that this can still be implemented somehow. Basically the move plugin defines a custom handler that re-creates the page content by taking most parsed syntax bits unmodified and changing a few. It would be great if something like this was still possible without writing custom versions of every syntax class. I have no problem with changing stuff, but it would be nice if you could keep this use case in mind when re-designing the parser/handler structure. Thank you!

@splitbrain

This comment has been minimized.

Owner

splitbrain commented Apr 30, 2018

@michitux thanks for the heads up - I'll keep that in mind.

adjusted Doku_Parser for PSR-2
All properties are declared protected. The handler is now set via the
constructor.
fix numericopt setting
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
@Chris--S

This comment was marked as resolved.

Collaborator

Chris--S commented on f37fd93 May 25, 2018

👍

splitbrain and others added some commits May 25, 2018

show other errors in undefined settings again
This reestablishes the mechanism of adding errors as Sepcial classes to the
undefined list.
fallback classes for plugins inheriting from old settings classes
This will still throw a signature mismatch warning on PHP7 but at least
it is no longer fatal.
do not initialize the configuration in constructor
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.
Merge pull request #2382 from splitbrain/psr2-config
PSR-2 refactoring for config plugin
Merge branch 'master' into psr2
* master:
  🐛 (Draft) Fix exception when actually viewing a draft of a page
  💡(Draft): Add doc block for constructor
   Add unittest for deleteUsers remote API call
  fix remote API call dokuwiki.deleteUsers
  translation update
  🏗 Use json for the response to dw_locktimer
  (dw_locktimer) plugins may reuse to add fields and callbacks
  💄 (editor) draft status is semantically not part of the toolbar
  🏗 Create new Draft class and move draft handling there
  translation update
  correctly avoid notice in init.
  avoid creating expensive stacktrace in dbg_deprecated()
  add method to EventHandler to check if an event is actually handled
  introduce INFO_DEPRECATION_LOG event
remove debugging stuff from detail.php
we haven't needed this in years
split changelog classes into their own namespace
The remaining functions in inc/changelog.php should be moved into a
utility class.
@splitbrain

This comment has been minimized.

Owner

splitbrain commented on 4da1351 Jun 21, 2018

@Chris--S are you sure you wanted to push this?

This comment has been minimized.

Collaborator

Chris--S replied Jun 21, 2018

This comment has been minimized.

Collaborator

Chris--S replied Jun 21, 2018

Reverted. I wonder if those files should be added to .gitignore ?

Chris--S and others added some commits Jun 21, 2018

fixed popularity plugin
The callback needs to be public
Merge branch 'master' into psr2
* master:
  upgraded JSON class to latest (2006) version
  continue is break in switch
  translation update
  reference existing proper progress gif. fixes #2441
  Fix missing ui-bg_glass_95_fef1ec_1x400.png and be/jquery.ui.datepicker.js for jquery
  removed accidental merges of outdated translations
  Change `const` use to `var` for Safari 9 (on iOS)
  Fix .htaccess files for Apache 2.4 (and 2.2)
  add logic if the server uses unlimited memory settings in is_mem_available()
  removed safemode hack
deprecated JSON class
JSON is natively supported since years. This makes
2145bd4 obsolete again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment