Navigation Menu

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

Merged
merged 185 commits into from Oct 10, 2019
Merged

PSR 2 Adoption #2358

merged 185 commits into from Oct 10, 2019

Conversation

splitbrain
Copy link
Collaborator

@splitbrain 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 --report-width=1000 --no-colors |grep -vF ' [x] ' |grep -E '^( +[0-9]|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.

Current Status: 68 problems left

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
We have to support lots of legacy code without namespaces
because backwards compatibility. There will be more excludes. This is
just a start.
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.
First start at declaring visibilites for methods and properties. Still
missing: the parser/renderer stuff and the plugins
They are now proper abstract classes
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. '.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

This comment was marked as resolved.

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.
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.
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 has been minimized.

@splitbrain

This comment has been minimized.

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.
* 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
Copy link
Collaborator Author

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
Copy link
Collaborator

I'll take a look.

@lpaulsen93
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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

All properties are declared protected. The handler is now set via the
constructor.
micgro42 and others added 24 commits April 22, 2019 21:32
This should better adhere to SRP and simplify things.
This class has been replaced by the classes in the
dokuwiki/Subscriptions namespace.
* psr2-pluginredux:
  Minor optimizations in PluginController
  Snake to Camel case fixes inn PluginController
  Fix snake->camel case, doc blocks
  minor code simplification
  snake to camel case fixes in EventHandler
  Move list of plugin types to plugin controller constant
  Avoid accessing the evet system before it's intialized
  Avoid processing events before the Event System is intiialized
  isEnabled instead of isDisabled
  removed get_directory() method from PluginController
  fix type hints
  moved plugin controller to Extension namespace
  removed deleted file from autoloader
  deprecated trigger_event() in favor of a static method on Event
  First go at moving the plugin classes into their own namespace
We still need to ignore a bunch of stuff for backwardscompatibility
Subscriptions are autoloaded now
We define and register the autoloader here.
These will be loaded via include, later on and should be cachable by
Op-Cache.

The formatting has been adjusted to have one entry per line to make
diffing much easier in the future.

For now duplicate keys and commented code from the originals have been
kept. But this should probably be cleaned up in the future.

For now these tables are not used, yet.
This doesn't really change much since the old functions are still needed
for compatibility reasons. We may be able to reduce the number of
functions by checking which ones we really need.
Docblocks, imports, etc...
mostly overlong lines and more exclude patterns
Actually replacing all calls is still to come.
also avoids overlong line
For now this uses full qualified namespaces, sensible imports may come
later.
* master: (34 commits)
  fix color for noninstalled extensions
  show disabled extensions in gray
  warn about inaccessible repo api
  bugfix: access check was never cached
  First go on a CLI component for the extension manager
  use strict type comparison
  translation update
  translation update
  fix #dokuwiki__sitetools current item not in highlight due to Greebo change
  authplain: Add tests for group retrieval
  authplain: Add a simple method for retrieving user groups
  translation update
  Negative string offsets are allowed in PHP 7.1+ only
  improve memory check output
  fix and test php_to_byte() related to #2756 #2556
  translation update
  translation update
  translation update
  translation update
  translation update
  ...
This class needs to be replaced with dokuwiki\Form in the future.
* utf8refactor:
  replaced deprecated utf8 functions
  formatting cleanup
  mark old utf8 functions deprecated
  Some cleanup for the UTF-8 stuff
  Moved all utf8 methods to their own namespaced classes
  Create separate table files for UTF-8 handling
We replace original PHP methods here and the original method is in
snake_case, so it makes sense to keep it this way even though that
violates PSR-2
changes from commit b15f23f may have
been lost, because the code changed to much.

changes from commit df81ca9 need to be
verified. we might need to do some more adjustments.

* master: (49 commits)
  translation update
  translation update
  translation update
  translation update
  translation update
  translation update
  translation update
  Update config.class.php
  translation update
  translation update
  Update lang.php
  Add ugc hint to nofollow
  add UGC hint
  translation update
  translation update
  use a script to fetch the correct phpunit
  Revert "output travis php version for easier debugging"
  setup databases for unit tests in travis
  output travis php version for easier debugging
  translation update
  ...
@splitbrain splitbrain merged commit c0c77cd into master Oct 10, 2019
micgro42 added a commit that referenced this pull request Jan 14, 2020
Apparently, #2358 introduced a breaking change to the constructor and
usage of Doku_Parser, which broke some plugins, e.g. the move plugin in
michitux/dokuwiki-plugin-move#176

This patch should restore the legacy behavior of this deprecated class.
@micgro42 micgro42 deleted the psr2 branch January 15, 2020 22:21
micgro42 added a commit that referenced this pull request Jan 15, 2020
Apparently, #2358 introduced a breaking change to the constructor and
usage of Doku_Parser, which broke some plugins, e.g. the move plugin in
michitux/dokuwiki-plugin-move#176

This patch should restore the legacy behavior of this deprecated class.
micgro42 added a commit that referenced this pull request Jan 22, 2020
Apparently, #2358 introduced a breaking change to the constructor and
usage of Doku_Parser, which broke some plugins, e.g. the move plugin in
michitux/dokuwiki-plugin-move#176

This patch should restore the legacy behavior of this deprecated class.
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

6 participants