Pico 1.0 #252

Merged
merged 120 commits into from Nov 6, 2015

Projects

None yet

8 participants

@PhrozenByte
Collaborator

First of all: Why? The sourcecode of existing forks like BaunCMS and PhilleCMS isn't "stupidly simple" anymore... You can read Picos sourcecode from top to down and even copy&paste programmers will understand what's going on. It's all about "understanding at first glance".

Most important second: All changes are 100% backward compatible.

I considered writing something own, but then caught up with Pico. The only thing missing was clean code - Picos concept, workflow and code really is "stupidly simple", but very powerful. Actually I just did a code refactoring. I don't want to fork Pico and I won't do it even if you reject my changes (obviously I'll still use it myself 😄).

This should be v1.0 ready. I recommend to release it as v1.0-beta, waiting some weeks to test it on a large user basis (I'll fix all appearing problems) and finally releasing it as Pico 1.0 😄

Please give me a hint if you'll merge this, I'll then update the homepage (gh-pages branch), changelog.txt etc. accordingly.

What did I do?

  • Fixes #79, Closes #93, Fixes #99, Closes #103, Closes #116, Fixes #140, Fixes #158, Closes #171, Fixes #210, Closes #241, Closes #244, Closes #246, Fixes #248, Fixes #249, Fixes #250, Fixes #251, Fixes #253, Fixes #257, Fixes #267
  • The code is now documented, code styling follows PSR. Pico superficially grows from 400 LoC to 1000 LoC, when removing all comments, Pico grows from 300 LoC to 500 LoC, mostly because of new public getter and helper methods (= 100 LoC) and PSR code styling. As said, there are no big functional changes, it's more a code refactoring.
  • The new routing system now works out-of-the-box (even without rewriting) with any webserver using the QUERY_STRING routing method. Internal links now look like ?sub/page, rewriting to basically remove the ? is still possible and recommended. Contrary to Pico 0.9 every webserver should work just fine. Pico 0.9 required working URL rewriting, so if you want to use old plugins/themes/contents, a working rewrite setup may still be required. If you're not using the default .htaccess, you must update your rewrite rules to follow the new principles. Internal links in content files are declared with %base_url%?sub/page. Internal links in templates should be declared using the new link filter (e.g. {{ "sub/page"|link }}), what basically calls Pico::getPageUrl(). You musn't change anything if you setup rewriting (what was required in Pico 0.9...), so I assume this to be fully backward compatible 😄
  • I've implemented a whole new plugin system while maintaining full backward compatibility. See the class docs of IPicoPlugin for details. The new event system supports plugin dependencies as well as some new events. It was necessary to reliably distinct between old and new events, so all events were renamed. The new PicoDeprecated plugin is crucial for backward compatibility, it's enabled on demand. Refer to its class docs for details.
  • I completely overhauled the sorting algorithms, so sorting by date now works as expected by using timestamps and index files are listed before their sub pages (so e.g. sub/index.md is listed before sub/foo.md).
  • Inaccessible files (like sub.md while sub/index.md exists) are now excluded from the pages list.
  • Now supporting per-directory 404.md files (if there is a sub/404.md, this file is displayed in favour of the global 404.md file, when e.g. the non-existing sub/notexisting.md is requested)
  • The file extension of content files is now configurable.
  • Heads up! #158 is fixed only when the PicoParsePagesContent plugin isn't enabled. It's disabled by default, but gets automatically enabled with PicoDeprecated as soon as an old plugin is loaded. This is necessary to maintain backward compatibility. You can still disable it manually by executing $pico->getPlugin('PicoParsePagesContent')->setEnabled(false); or adding $config['PicoParsePagesContent.enabled'] = false; to your config.php.
  • The meta headers are now parsed by the YAML component of the Symfony project (see #116) and it isn't necessary to register new headers during the onMetaHeaders event anymore. The implicit availability of headers is supposed to be used by users and pure (!) theme developers only, so we're still instructing plugin developers to register their headers. See the commit message of 7537159 for more detail.
  • Meta header variables are now accessible in content files using %meta.*%, so you mustn't repeat yourself. You can now put an excerpt into the description meta variable and output the same content at the beginning of the article using %meta.description% (see index.md for a example).
  • Initialization of Pico now works completely different: rather than defining constants (which are probably conflicting with other applications...), Pico now expects its paths to be passed as parameters to the constructor. The constructor doesn't start Picos processing anymore, you now have to call the Pico::run() method, which returns the parsed page contents instead of directly echoing them. The PicoDeprecated plugin defines the now deprecated constants ROOT_DIR, LIB_DIR etc., so old plugins can still use them. Those constants are defined before reading config.php in Picos root folder, so upgrading users usually aren't bothered with e.g. a PHP Notice: Use of undefined constant ROOT_DIR - assumed 'ROOT_DIR' error when using ROOT_DIR in their config.php (so: no BC break). New users don't need the constants anymore, relative paths are now always interpreted to be relative to Picos root dir, so $config['content_dir'] = 'content'; is always sufficient (previously this was depending on the webserver config). All these changes are supposed to improve Picos interoperability with other applications and allows developers to integrate Pico in other applications, therefore I additionally added a new Pico::setConfig() method to even make the use of a config.php optional.
  • The Twig cache dir doesn't exist anymore. The provided directory never was helpful, in most cases one still had to make the directory writable for the webserver, so it never worked out-of-the-box.
  • All index.html files were removed; instead of helping users to secure their website, they create a false sense of security. This leads to situations like "I'm receiving a empty website when navigating to the content folder, thus everything must be safe!"
  • I decided explicitly to NOT implement pages as objects ("stupidly simple", see above). Anyway, I think plugin developers shouldn't manipulate data in "wrong" events, this could lead us to unexpected behaviour. Sure, plugin developers still can do this, we're passing variables by reference, but it's not that obvious. I even thought about dereferencing the values after the corrosponding event was called, but that would be backward incompatible. What do you think?
  • How to fix the "composer problem" discussed in #221 and #223? There's a very simple solution: When creating a release on GitHub (after you've pushed the tag) you can upload "binaries". Simply execute composer locally, create a ZIP archive and upload the result as "binary".
  • I didn't care much about #110, #238, #239 and #240 because I simply don't need these features. But I think they are good ideas and the core should support this. Just my 2 cents 😄
  • #201 and #231 should be closed - this can easily be achieved with plugins. In fact, there are already plugins adding support for these features...
  • Imo distinct documentations for users, theme designers and plugin devs is MUCH more important than unit tests... Pico is a project with just about 500 LoC (+ comments), such a manageable project doesn't necessarily require unit tests - they are nice to have, but that's it. Documentation should be the top priority!
  • Note: I'm no english native speaker. Maybe someone should look through my code comments 😄
@theshka
Collaborator
theshka commented Aug 30, 2015

Great work! I would definitely like to push this upstream-- it's so fresh, so clean 👍

Just a couple small problems to fix straight away:

In global.php there's some funky paths still set to your environment; replacing the first two lines, and THEMES_DIR path does the trick:

//define('HTTPDOCS', realpath(rtrim(__DIR__, '/')) . '/');
//define('ROOT_DIR', realpath(HTTPDOCS . '../httpdocs-includes') . '/');
define('ROOT_DIR', realpath(rtrim(__DIR__, '/')) . '/');

//define('THEMES_DIR', HTTPDOCS . 'themes/');
define('THEMES_DIR', ROOT_DIR . 'themes/');

and line 377 of pico.php throws a NOTICE when $_SERVER['QUERY_STRING'] is null.

//$pathComponent = $_SERVER['QUERY_STRING'];
$pathComponent = (isset($_SERVER['QUERY_STRING']) ? $_SERVER['QUERY_STRING'] : '');

I'd like to get some more input, anyone else still following Pico??

@theshka theshka added this to the Version 1.0 milestone Aug 30, 2015
@PhrozenByte
Collaborator

Oops, that was the wrong global.php 😄
Sure, it's a good idea to wait for about 1-2 weeks to get a little more feedback before merging this, it's a quite big update

@fourtonfish

When ordering posts by date, would it make more sense to use something like this?

  if ($order_by == 'date' && isset($page_meta['date'])) {
      $sorted_pages[$page_meta['date_formatted'] . $date_id] = $data;
      $date_id++;
  } else {

Currently it's this.

      $sorted_pages[$page_meta['date'] . $date_id] = $data;

This seems to be the only thing that works for me, as I'm using the date format of MMMM DD, YYYY (eg August 31, 2015).

@fourtonfish fourtonfish referenced this pull request in botwiki/botwiki.org Sep 5, 2015
Closed

Sort bots by (Last Modification) date #11

@PhrozenByte
Collaborator

I'm not really sure what you mean. In this PR posts aren't sorted by the Date meta header directly, instead they are sorted by $data['time'], the timestamp resulting from a strtotime() call. What sorting would you expect?

@fourtonfish

@PhrozenByte

Well, if you go to https://botwiki.org/tag/twitterbot, the posts are now correctly ordered while using the MMMM DD, YYYY format (see this for an example).

If I go back to using $page_meta['date'] for sorting, it seems almost random.

Feel free to clone https://github.com/botwiki/botwiki.org and play with it locally.

@PontusHorn

This looks great to me. Would be more than happy to see it merged.

@fourtonfish, have you looked through the changes in the PR? The code you're referring to isn't used anymore in it. Your problem, as I understand it, is already fixed here.

@fourtonfish

@PontusHorn Ah, my bad, I think I only looked at the most current stable, not this PR.

Well, great job, then :-)

@bricebou
bricebou commented Sep 6, 2015

Hi,

First of all, I'm not a developer and english is not my native language ; but it's great to see Pico under active development !

I don't understand all the implications of this pull request and I can't evaluate your code, but, based on some comments, it seems great. And it's coherent with the purpose of Pico : simple. And I agree with a proposition that have been already made : Pico should be available as an archive to keep its installation simple and the more wide as possible (you don't always have an SSH access to perform composer on shared hosting).

I've written some small plugins for Pico and I was wondering what would be necessary to make them coherent with the changes you made. I'd like them to be fully compatible with this new version, and not use the deprecated "way".

Would it be possible for someone here to review my code and give me some advice, recommendations... to perform an update.
Here are the repositories:

Thanks in advance and thanks for all the work @PhrozenByte, @theshka and others made !

@PhrozenByte
Collaborator

@bricebou, sure, that's pretty easy. The most important changes are regarding the event system. You can use the transition table at the PicoDeprecated class docs to see the new event names. Some events changed their parameters to be more consistent, so you should compare them to the sourcecode of DummyPlugin (formerly Pico_Plugin) - or simply use the list below. Then add extends AbstractPicoPlugin to your class and you're done 😃

If you're using internal links to other pages, you shouldn't use $settings['base_url'] anymore. Use $this->getPageUrl($page) instead, otherwise your plugin requires a working URL rewriting setup. I recommend to delete the .htaccess file while developing, that's the easiest way to guarantee support of the new routing system.

It's recommended, but not required, that you rename your classes to follow the PSR principles, so e.g. Pico_Leaflet becomes PicoLeaflet and the plugin file name becomes PicoLeaflet.php.

Here's a list of changes to the event system that are relevant for existing plugins:

  • All events were renamed - see the transition table at the PicoDeprecated class docs for details
  • You can access all Pico variables using public getter methods, so instead of public function request_url(&$url) { $this->url = $url; } to remember the requested URL you can simply use $this->getRequestUrl()
  • onContentLoaded and on404ContentLoaded dropped $requestFile - use $this->getRequestFile() instead
  • onSinglePageLoaded dropped $meta - use $pageData['meta'] instead
  • onSinglePageLoaded passes $pageData indexed by ID - use array_values($pageData) when necessary
  • onPageRendering changed its parameter order to $twig, $twigVariables, $templateName
  • onPageRendering now passes $templateName with its file extension
@PhrozenByte PhrozenByte Access plugins by class name, not file name
Class name and file name can differ regarding case sensitivity
a83b01e
@bricebou
bricebou commented Sep 6, 2015

Thanks for these precisions. I'll look into it, it will be my second semester's hobby...

If I understand correctly, each plugin should have its own config/config.php file ?

@PhrozenByte
Collaborator

No, I didn't change anything regarding configuration, it's still all inside a single config/config.php.

@all
There are some "What do you think?" questions in the PR message, the most important about the need to register new meta header variables during onMetaHeaders. It would be nice to get a little feedback about that, I'm currently tending to remove the need to register new meta variables (as it is required in Pico 0.9). Registering a meta variable still guarantees the existance of the array key, so you don't need isset() checks. It also still allows plugin developers to access a meta variable through another array key.

@bricebou
bricebou commented Sep 6, 2015

Ah ok, I've misunderstood lines 36 to 38 in https://github.com/PhrozenByte/Pico/blob/pico1.0/plugins/00-PicoDeprecated.php

Thanks again.

@PontusHorn

@PhrozenByte I think it's reasonable to require meta headers to be registered, as it makes the usage of the header more predictable. Not a huge issue either way though.

@PontusHorn PontusHorn commented on an outdated diff Sep 13, 2015
+ $templateName .= '.html';
+ }
+
+ $this->triggerEvent('onPageRendering', array(&$this->twig, &$this->twigVariables, &$templateName));
+
+ $output = $this->twig->render($templateName, $this->twigVariables);
+ $this->triggerEvent('onPageRendered', array(&$output));
+
+ echo $output;
+ }
+
+ /**
+ * Loads plugins from PLUGINS_DIR in alphabetical order
+ *
+ * Plugin files may be prefixed by a number (e.g. 00-PicoDeprecated.php)
+ * to indicate their processsing order. You MUST NOT use prefixes between
@PontusHorn
PontusHorn Sep 13, 2015

Typo in "processing".

@PontusHorn PontusHorn commented on an outdated diff Sep 13, 2015
+
+ $this->triggerEvent('onMetaHeaders', array(&$headers));
+ return $headers;
+ }
+
+ /**
+ * Parses the file meta from raw file contents
+ *
+ * Meta data MUST start on the first line of the file, either opened and
+ * closed by --- or C-style block comments (deprecated). The headers are
+ * parsed by the YAML component of the Symfony project. You MUST register
+ * new headers during the `onMetaHeaders` event first, otherwise they are
+ * ignored and won't be returned.
+ *
+ * @see <http://symfony.com/doc/current/components/yaml/introduction.html>
+ * @param string $content the raw file contents
@PontusHorn
PontusHorn Sep 13, 2015

Wrong parameter name - should be $rawContent

@PontusHorn PontusHorn commented on an outdated diff Sep 13, 2015
+ * Returns the URL to a given page
+ *
+ * @param string $page identifier of the page to link to
+ * @return string URL
+ */
+ public function getPageUrl($page)
+ {
+ return $this->getBaseUrl() . ((!$this->isUrlRewritingEnabled() && !empty($page)) ? '?' : '') . $page;
+ }
+
+ /**
+ * Recursively walks through a directory and returns all containing files
+ * matching the specified file extension in alphabetical order
+ *
+ * @param string $directory start directory
+ * @param string $ext return files with this file extension only (optional)
@PontusHorn
PontusHorn Sep 13, 2015

Wrong parameter name - should be $fileExtension

@PontusHorn PontusHorn commented on an outdated diff Sep 13, 2015
+
+ // scandir() reads files in alphabetical order
+ $files = scandir($directory);
+ $fileExtensionLength = strlen($fileExtension);
+ if ($files !== false) {
+ foreach ($files as $file) {
+ // exclude hidden files/dirs starting with a .; this also excludes the special dirs . and ..
+ // exclude files ending with a ~ (vim/nano backup) or # (emacs backup)
+ if ((substr($file, 0, 1) === '.') || in_array(substr($file, -1), array('~', '#'))) {
+ continue;
+ }
+
+ if (is_dir($directory . '/' . $file)) {
+ // get files recursively
+ $result = array_merge($result, $this->getFiles($directory . '/' . $file, $fileExtension));
+ } elseif (empty($fileExtension) || (substr($file, -strlen($fileExtension)) === $fileExtension)) {
@PontusHorn
PontusHorn Sep 13, 2015

Should use the $fileExtensionLength variable defined in line 901 instead of the strlen() call.

@PontusHorn PontusHorn commented on an outdated diff Sep 13, 2015
lib/AbstractPicoPlugin.php
+ * @return void
+ * @throws RuntimeException thrown when a dependency fails
+ */
+ protected function checkDependants($recursive)
+ {
+ $dependants = $this->getDependants();
+ if (!empty($dependants)) {
+ if ($recursive) {
+ foreach ($this->getDependants() as $pluginName => $plugin) {
+ if ($plugin->isEnabled()) {
+ if (!$plugin->isStatusChanged()) {
+ $plugin->setEnabled(false, true, true);
+ } else {
+ throw new RuntimeException(
+ "Unable to disable plugin '" . get_called_class() . "': "
+ + "Required by manually enabled plugin '" . $pluginName . "'"
@PontusHorn
PontusHorn Sep 13, 2015

Wrong string concatenation operator

@PontusHorn PontusHorn commented on an outdated diff Sep 13, 2015
lib/AbstractPicoPlugin.php
+ if ($recursive) {
+ foreach ($this->getDependants() as $pluginName => $plugin) {
+ if ($plugin->isEnabled()) {
+ if (!$plugin->isStatusChanged()) {
+ $plugin->setEnabled(false, true, true);
+ } else {
+ throw new RuntimeException(
+ "Unable to disable plugin '" . get_called_class() . "': "
+ + "Required by manually enabled plugin '" . $pluginName . "'"
+ );
+ }
+ }
+ }
+ } else {
+ $dependantsList = 'plugin' . ((count($dependants) > 1) ? 's' : '') . ' ';
+ $dependantsList .= "'" + implode("', '", array_keys($dependants)) + "'";
@PontusHorn
PontusHorn Sep 13, 2015

Wrong string concatenation operator

@PontusHorn PontusHorn commented on an outdated diff Sep 13, 2015
lib/AbstractPicoPlugin.php
+ if (!$plugin->isStatusChanged()) {
+ $plugin->setEnabled(false, true, true);
+ } else {
+ throw new RuntimeException(
+ "Unable to disable plugin '" . get_called_class() . "': "
+ + "Required by manually enabled plugin '" . $pluginName . "'"
+ );
+ }
+ }
+ }
+ } else {
+ $dependantsList = 'plugin' . ((count($dependants) > 1) ? 's' : '') . ' ';
+ $dependantsList .= "'" + implode("', '", array_keys($dependants)) + "'";
+ throw new RuntimeException(
+ "Unable to disable plugin '" . get_called_class() . "': "
+ + "Required by " . $dependantsList
@PontusHorn
PontusHorn Sep 13, 2015

Wrong string concatenation operator

@PontusHorn PontusHorn commented on an outdated diff Sep 13, 2015
plugins/DummyPlugin.php
+ * Triggered after Pico prepared the raw file contents for parsing
+ *
+ * @see Pico::parseFileContent()
+ * @param string &$content prepared file contents for parsing
+ * @return void
+ */
+ public function prepareFileContent(&$content)
+ {
+ // your code
+ }
+
+ /**
+ * Triggered after Pico parsed the contents of the file to serve
+ *
+ * @see Pico::getFileContent()
+ * @param stirng &$content parsed contents
@PontusHorn
PontusHorn Sep 13, 2015

Typo in "string"

@PhrozenByte PhrozenByte Various fixes
Thanks @PontusHorn for spotting!
71e7da2
@PhrozenByte
Collaborator

Thanks @PontusHorn

@theshka: I suggest to wait 1-2 more weeks, I'm still hoping for more feedback (+ I currently don't have time to update the docs 😄)

@all: Feedback is still appreciated!

@dav-m85 dav-m85 and 2 others commented on an outdated diff Sep 14, 2015
composer.json
@@ -14,9 +14,14 @@
"require": {
"php": ">=5.3.2",
"twig/twig": "1.18.*",
- "erusev/parsedown-extra": "dev-master@dev"
+ "erusev/parsedown-extra": "dev-master@dev",
@dav-m85
dav-m85 Sep 14, 2015 Contributor

Mh why not a stable constraint here ? Gonna break at the next composer update.

@PhrozenByte
PhrozenByte Sep 14, 2015 Collaborator

@theshka (968dc18), are there any reasons that oppose using e.g. 0.7.* instead?

@theshka
theshka Sep 14, 2015 Collaborator

@PhrozenByte, @dav-m85 ... No, I think that was just overlooked at some point. I would say to use a stable constraint here instead of the dev branch.

@dav-m85 dav-m85 and 2 others commented on an outdated diff Sep 14, 2015
lib/IPicoPlugin.php
+ * advantage from this behaviour if you want to do something only when old
+ * plugins are loaded. Consequently the old events are never triggered when
+ * your plugin is implementing this interface and no old plugins are present.
+ *
+ * If you're developing a new plugin, you MUST implement IPicoPlugin. If
+ * you're the developer of an old plugin, it is STRONGLY RECOMMENDED to use
+ * the events introduced in Pico 1.0 when releasing a new version of your
+ * plugin. If you want to use any of the new events, you MUST implement
+ * IPicoPlugin and update all other events you use.
+ *
+ * @author Daniel Rudolf
+ * @link http://picocms.org
+ * @license http://opensource.org/licenses/MIT
+ * @version 1.0
+ */
+interface IPicoPlugin
@dav-m85
dav-m85 Sep 14, 2015 Contributor

Been a while I haven't seen hungarian notation. I would suggest considering the sf2 usage, tht would be PicoPluginInterface

@PhrozenByte
PhrozenByte Sep 14, 2015 Collaborator

I personally prefer prefixing interfaces/traits with I/T towards adding Interface/Trait - but that's nothing I insist on. More opinions are appreciated.

@theshka
theshka Sep 14, 2015 Collaborator

Out of my depth with this one, and have no strong preference either way; however, PicoPluginInterface sounds more natural to me than IPicoPlugin. 🙊

@dav-m85
Contributor
dav-m85 commented Sep 14, 2015

Nice work @PhrozenByte. That's a LOT of work you put it here, I've been myself tempted of forking but always get back to the source. If you're sure the changes does not introduces any BC breaks, I would suggest sticking to semver and just increment the minor version.

Also I find really difficult to review all changes in one single PR, everything is mixed. It would be IMHO simpler to split the PR in multiple sub components and submit them to a temporary 0.9-dev branch. @theshka ?

I would consider switching to 1.0 branch once we got some testing coverage and definitely remove the stuff you introduce as deprecated in this PR.

@fourtonfish fourtonfish referenced this pull request Sep 14, 2015
Closed

Problem with HTTPS #253

@Lomanic
Lomanic commented Sep 14, 2015

Hi, thank you for your work on Pico @PhrozenByte !

Unfortunately, I tried to install your pico1.0 branch with PHP 5.3.3-7 and Apache with mod-rewrite enabled like below with the following result when testing in the browser:

Fatal error: Can't use method return value in write context in /var/home/XXX/www/picocms4/lib/Pico.php on line 838 

Steps I used to install:

cd www
wget 'https://codeload.github.com/PhrozenByte/Pico/zip/pico1.0' -O 'Pico-pico1.0.zip'
unzip Pico-pico1.0.zip
mv Pico-pico1.0 picocms4
cd picocms4
curl -sS https://getcomposer.org/installer | php
php composer.phar install

I just replaced this problematic line by if ($this->getConfig('base_url')) { following this SO answer and then it complained that there were no config/config.php file to include, so I copied the config/config.php.template file to config/config.php, and now there are a lot more errors when testing in browser:

Notice: Undefined variable: config in /var/home/XXX/www/picocms4/config/config.php on line 61
Warning: DOMDocument::saveHTML() expects exactly 0 parameters, 1 given in /var/home/XXX/www/picocms4/vendor/erusev/parsedown-extra/ParsedownExtra.php on line 492
Warning: DOMDocument::loadHTML(): Empty string supplied as input in /var/home/XXX/www/picocms4/vendor/erusev/parsedown-extra/ParsedownExtra.php on line 471
Catchable fatal error: Argument 1 passed to DOMNode::removeChild() must be an instance of DOMNode, null given, called in /var/home/XXX/www/picocms4/vendor/erusev/parsedown-extra/ParsedownExtra.php on line 496 and defined in /var/home/XXX/www/picocms4/vendor/erusev/parsedown-extra/ParsedownExtra.php on line 472

Was PHP 5.3.3 support dropped or is it something else? I see in composer.json that it requires "php": ">=5.3.2", so it should be OK, am I missing some steps, some doc? Should I and could I open an issue for this pull request?

@PhrozenByte
Collaborator

No @Lomanic, you're not missing anything, you just found some bugs which should be fixed now, please try again 😃

@dav-m85: These are quiet big changes, so, in my opinion, releasing them as Pico 1.0 is justifiable. Please remember that Pico never had a stable release - all previous releases were pre-releases (0. versions), this actually would be the first stable release. There are no BC breaks, but the changes regarding the plugin system are enormous. I even recommend to keep the PicoDeprecated plugin for a very long time (e.g. until Pico 2.0). Pico already has a ecosystem that depends on these deprecated features, releasing a new, incompatible version cuts Pico off from its own ecosystem. I agree that at least auto-enabling the PicoParsePagesContent plugin should be removed with the next bigger release (e.g. Pico 1.1) - this plugin really has a big performance impact (as Pico 0.9 had...).

@Lomanic
Lomanic commented Sep 14, 2015

Thanks @PhrozenByte for the quick fix!

I changed the two modified files (config/config.php.template and lib/Pico.php), copied config.php.template to config.php and got these errors:

Warning: DOMDocument::saveHTML() expects exactly 0 parameters, 1 given in /var/home/XXX/www/picocms4/vendor/erusev/parsedown-extra/ParsedownExtra.php on line 492
Warning: DOMDocument::loadHTML(): Empty string supplied as input in /var/home/XXX/www/picocms4/vendor/erusev/parsedown-extra/ParsedownExtra.php on line 471
Catchable fatal error: Argument 1 passed to DOMNode::removeChild() must be an instance of DOMNode, null given, called in /var/home/XXX/www/picocms4/vendor/erusev/parsedown-extra/ParsedownExtra.php on line 496 and defined in /var/home/XXX/www/picocms4/vendor/erusev/parsedown-extra/ParsedownExtra.php on line 472

After some research, it looks like Parsedown-extra uses DOMDocument::savehtml() with the $node parameter, and so we need PHP 5.3.6 (I have 5.3.3 ; see savehtml()'s Changelog). You will have to raise the php version requirement in composer.json (Parsedown-extra should do it itself, I'm surprised it does not include a minimum php version in its own composer.json). I will perhaps downgrade Parsedown-extra until it does not use this function (this is just for testing, I will then migrate to a better updated environment). I will report which version works with php 5.3.3.
Edit: Parsedown-extra 0.3.0 just gives warnings on php 5.3.3. Next step for me is to test my plugins and template against your pico1.0 branch, stay tuned 😉.

@PhrozenByte
Collaborator

@Lomanic: Can you please report this issue to erusev/parsedown-extra? Maybe they'll fix this problem before we merge this PR.

@Lomanic Lomanic referenced this pull request in erusev/parsedown-extra Sep 15, 2015
Open

DOMDocument::saveHTML() with $node parameter on php < 5.3.6 #75

@Lomanic
Lomanic commented Sep 16, 2015

OK, I migrated my old pico (v0.8) to your v1.0 fork install, moved my plugins, moved my theme, moved my old config.php to config/config.php.

To migrate from my previous pico version (v0.8 I think) I copied my old config.php to config/config.php.template and added return $config; at the end of the file, otherwise I just had the default configuration. I copied my theme and my plugins to the right dirs and so far my theme is not broken. Unfortunately, I don't have excerpts of my articles on the default page (as a blog), with the {{ page.excerpt }} twig var.

Tested plugins:

  • Lomanic/Pico-Editor-Plugin : can login but all the paths with slashes are broken
  • rewdy/Pico-Pagination : paths with slashes are broken, but listing on the 1st page works
  • pico_search is broken for the same reason
@PhrozenByte
Collaborator

@Lomanic, excerpts were removed by default, you can re-enable them by adding $config['PicoExcerpt.enabled'] = true; to your config.php. PicoExcerpt depends on PicoParsePagesContent, what eliminates all performance advantages of Pico 1.0. You should move the excerpt contents to the Description meta header and add %meta.description% to the beginning of your content file. By the way, PicoExcerpt gets automatically enabled when a old plugin is loaded, thus this is no BC break.

You moved your config.php to config/config.php.template? This should disable your custom configuration and Pico falls back to its default configuration.

Can you please provide a copy of your website for debugging? Simply leave your contents directory out. Please send me an email through the contact form at http://daniel-rudolf.de/contact.

@PontusHorn

Using meta.description for excerpts is fair enough, but it might be a good idea to run it through Twig's striptags filter when printing it in <meta name="description" />, to allow for some basic formatting of the excerpt.

@fourtonfish fourtonfish referenced this pull request in botwiki/botwiki.org Sep 26, 2015
Open

"Edit me" buttons #26

@PhrozenByte
Collaborator

@Lomanic, I casually found (and fixed) a bug in PicoDeprecated that could lead to your problems with 3rd party plugins, but I can't confirm that without a copy of your Pico installation. Please try it yourself.

@all, I'll update the docs in the course of this week, so I would like to call you up for testing and further feedback. @theshka, I then recommend to merge this PR in the next week, this unfortunately seems to be the only way to get the necessary attention to receive more feedback.

@PontusHorn

I've been using this for two small sites for about two weeks without any issues. No errors logged despite regular traffic. I'm using two plugins of my own making which have been updated to work with this version - haven't done any testing worth mentioning with plugins that haven't been updated.

@dav-m85 dav-m85 and 1 other commented on an outdated diff Sep 28, 2015
@@ -1,15 +1,3 @@
<?php
-
-define('ROOT_DIR', realpath(dirname(__FILE__)) . '/');
@dav-m85
dav-m85 Sep 28, 2015 Contributor

What is the purpose of moving those constants in a separat file ? Why not moving them to the Pico class constructor for example ?

@PhrozenByte
PhrozenByte Sep 28, 2015 Collaborator

The ROOT_DIR, LIB_DIR and VENDOR_DIR constants are required before calling the constructor, so it's impossible to move them there.

Splitting constant definition (global.php) and instantiation of Pico allows developers to integrate Pico into their own ecosystem and overwrite the constants as necessary. It allows e.g. the following:

<?php

class MyApplication {
    protected $pico;

    // snip

    protected function initPico() {
        // either include the global.php
        require_once(PICO_DIR . 'global.php');
        // or define the constants on your own

        $this->pico = new Pico();
    }

    // snip
}

Anyway, this is no good solution at all, I focused too much on the existing code. I'll overhaul this, probably by replacing the constants with something better... Thanks for bringing this to my attention.

@PhrozenByte
PhrozenByte Oct 1, 2015 Collaborator

Please take a look at fc7632b and cdef7a6; any feedback?

@dav-m85
dav-m85 Oct 6, 2015 Contributor

Well that is roughly what I had in mind when spoting these constants. Looks tidy now :)

@PhrozenByte PhrozenByte and 2 others commented on an outdated diff Sep 29, 2015
+ {
+ return $this->content;
+ }
+
+ /**
+ * Reads the data of all pages known to Pico
+ *
+ * @return void
+ */
+ protected function readPages()
+ {
+ $pages = array();
+ $files = $this->getFiles($this->getConfig('content_dir'), $this->getConfig('content_ext'));
+ foreach ($files as $i => $file) {
+ // skip 404 page
+ if (basename($file) == '404' . $this->getConfig('content_ext')) {
@PhrozenByte
PhrozenByte Sep 29, 2015 Collaborator

Like in Pico 0.9, we exclude all 404.md files in any folder (e.g. sub/404.md). Pico 0.9 however always uses the "root" 404.md. What do you think about supporting directory-specific 404 files?

@PontusHorn
PontusHorn Sep 29, 2015

Personally, I don't see a particularly compelling use case that makes up for the added complexity that would bring.

@PhrozenByte
PhrozenByte Oct 1, 2015 Collaborator

One could want to use a different 404.md for his blog, e.g. showing a tag cloud, what wouldn't make much sense for non-blog-pages. Apart from the use case, should we then still exclude all 404.md files or just the "root" 404.md?

@PhrozenByte
PhrozenByte Oct 4, 2015 Collaborator

I think the added 10 LoC is it worth to resolve this undocumented and not expected behavior...

@bricebou
bricebou Oct 4, 2015

What is the unexpected behaviour you are referribg to?

Le dimanche 4 octobre 2015, Daniel Rudolf notifications@github.com a
écrit :

In lib/Pico.php
#252 (comment):

  • {
  •    return $this->content;
    
  • }
  • /**
  • \* Reads the data of all pages known to Pico
    
  • *
    
  • \* @return void
    
  • */
    
  • protected function readPages()
  • {
  •    $pages = array();
    
  •    $files = $this->getFiles($this->getConfig('content_dir'), $this->getConfig('content_ext'));
    
  •    foreach ($files as $i => $file) {
    
  •        // skip 404 page
    
  •        if (basename($file) == '404' . $this->getConfig('content_ext')) {
    

I think the added 10 LoC is it worth to resolve this undocumented and not
expected behavior...


Reply to this email directly or view it on GitHub
https://github.com/picocms/Pico/pull/252/files#r41097723.

Brice Boucard

@PhrozenByte
PhrozenByte Oct 4, 2015 Collaborator

See the commented line and 77f9390 😄

Pico 0.9 excluded all 404.md files in any directory from the pages list, even they weren't functional. This never was documented and is unexpected behavior - you either expect that only the global 404.md file is removed or that Pico supports per-directory 404.md files. The first solution would break BC, so I decided that the additional 10 LoC are it worth to resolve this problem.

@PhrozenByte PhrozenByte and 1 other commented on an outdated diff Sep 29, 2015
+ );
+
+ if ($file == $this->requestFile) {
+ $page['content'] = &$this->content;
+ }
+
+ unset($rawContent, $meta);
+
+ // trigger event
+ $this->triggerEvent('onSinglePageLoaded', array(&$page));
+
+ $pages[$id] = $page;
+ }
+
+ // sort pages by date
+ // Pico::getFiles() already sorts alphabetically
@PhrozenByte
PhrozenByte Sep 29, 2015 Collaborator

Like in Pico 0.9, files are strictly sorted in alphabetical order, so sub/bar.md and sub/foo.md are listed before sub/index.md. What do you think about making an exception for index files, so sub/index.md is sorted by sub instead?

@PontusHorn
PontusHorn Sep 29, 2015

This seems like a good improvement. I'd say it makes more sense in most cases. For other cases, there's always the plugin hook onPagesLoaded.

PhrozenByte added some commits Oct 1, 2015
@PhrozenByte PhrozenByte Overhaul init of Pico
This may break BC if you're using one of the now deprecated constants (e.g. ROOT_DIR)
fc7632b
@PhrozenByte PhrozenByte Explicitly treat relative paths to be relative to Picos root dir
This tempers the BC break, we can now recommend to simply remove the ROOT_DIR part
cdef7a6
@PhrozenByte PhrozenByte Drop inaccessible pages
e.g. drop sub.md if sub/index.md exists
95db5ba
@theshka
Collaborator
theshka commented Oct 1, 2015

@theshka, I then recommend to merge this PR in the next week, this unfortunately seems to be the only way to get the necessary attention to receive more feedback.

@PhrozenByte yes, I think so as well, this is looking excellent! ... The change log and docs will need updating still, yeah?

@PhrozenByte
Collaborator

@theshka: Yes, changelog and docs still need a update. I'm currently working on some more improvements and will then update the docs accordingly. I think this PR can be merged in the course of the next week - as long as nobody gives feedback that needs action (@all, feedback is still appreciated!). I think this isn't in a hurry, a week sooner or later isn't important.

@bricebou
bricebou commented Oct 4, 2015

@PhrozenByte sorry for the delay but I'd like to react to your comment #252 (comment)

Maybe I don't really understand what you say about registering new meta variables but I don't see how you don't need isset() checks. For example, with the Leaflet plugin or the Tag plugin, you still have to check if the variable is set for each page. But I may misunderstood... :/

@PhrozenByte
Collaborator

@Lomanic: Thank you for providing your Pico install! I've fixed all problems related to Pico - as far as I can tell everything else works now as expected. 😃

The remaining Warning: array_unshift() expects parameter 1 to be array, null given in .../vendor/erusev/parsedown-extra/ParsedownExtra.php on line 28 error is related to your downgraded parsedown-extra version. It seems that your downgraded parsedown-extra version is incompatible to the current version of parsedown. Newer versions of parsedown-extra check for parsedown >= 1.5.0, so you could try a 1.4.x release. Otherwise please refer to @erusev.

@bricebou: My wording was a little bit ambiguous. Starting with Pico 1.0 you don't need isset() checks anymore. Please see lib/Pico.php#L630 and lib/Pico.php#L642.

PhrozenByte added some commits Oct 4, 2015
@PhrozenByte PhrozenByte Declare undefined $plugins variable
Thanks @Lomanic
3f7b099
@PhrozenByte PhrozenByte Support per-directory 404.md files 77f9390
@PhrozenByte PhrozenByte Improve method docs of Pico::load404Content() 9aa62b4
@PhrozenByte PhrozenByte Improve README.md ef1a9e0
@PhrozenByte PhrozenByte Define deprecated constants before evaluating the config.php in Picos…
… root dir

This prevents E_NOTICEs when using e.g. ROOT_DIR in a old config.php, so upgrading users are usually not bothered with this BC break
2e15e11
@PhrozenByte PhrozenByte Update changelog.txt
The changelog only provides basic information about the enormous changes introduced with Pico 1.0-beta. Please refer to the (not yet written... 😄) UPGRADE section of the docs for details.
006afa5
@PhrozenByte PhrozenByte Fix code formatting 27d6946
@theshka
Collaborator
theshka commented Oct 4, 2015

@PhrozenByte could you chime in on this thread? bricebou/pico_ganalytics#1
I'm not sure what I'm dong wrong with respect to the enabled/disabled state of plugins. @bricebou

PhrozenByte added some commits Oct 4, 2015
@PhrozenByte PhrozenByte Fix method docs typo 79e2dac
@PhrozenByte PhrozenByte Cast AbstractPicoPlugin::$dependsOn to array
Plugin devs could come up with the idea of setting AbstractPicoPlugin::$dependsOn to a string (single dependency) or null (no dependencies)
4f1e866
@PhrozenByte PhrozenByte Move sorting of $pages from Pico::getPages() to Pico::sortPages() 9d518fd
@PhrozenByte PhrozenByte Support $config['<plugin name>']['enabled'] option
... as a alternative to $config['<plugin name>.enabled']; Thanks @theshka for giving this hint
46ef631
@PhrozenByte PhrozenByte Split PicoDeprecated::onConfigLoaded() into multiple methods 1cbf48a
@PhrozenByte PhrozenByte Remove the need to register headers during onMetaHeaders()
Why? I'm currently writing the user docs and I really have no idea how to explain this whole process in a non-technical way... It is very likely that a normal user wants to use custom tags and it would be absurd to tell him,that he should learn a programming language to do so. On the other hand, providing a copy-and-paste template makes the whole idea of explicitly registering headers worthless. The only reasonable solution is to remove the need to register headers.

Anyway, I think @PontusHorn is right to say that registering headers makes the whole system more predictable. So plugin developers are still instructed to register their meta headers during . We actually can't check and ensure this, but that's imho the best solution.
7537159
@PhrozenByte PhrozenByte Update changelog.txt for 7537159 7aa199d
@Lomanic
Lomanic commented Oct 5, 2015

@PhrozenByte I confirm that you solved the issues with my legacy themes and plugins (last commit tested: 27d6946), I just had Notices/Warnings that I could easily disable 👍 . Now I think I will have to upgrade the plugins I use so they don't rely on the 00-PicoDeprecated.php shim, I will probably wait for your branch to be merged in master.

@dav-m85 dav-m85 and 1 other commented on an outdated diff Oct 6, 2015
-define('LIB_DIR', ROOT_DIR . 'lib/');
-define('VENDOR_DIR', ROOT_DIR . 'vendor/');
-define('PLUGINS_DIR', ROOT_DIR . 'plugins/');
-define('THEMES_DIR', ROOT_DIR . 'themes/');
-define('CONFIG_DIR', ROOT_DIR . 'config/');
-define('CACHE_DIR', LIB_DIR . 'cache/');
-
-define('CONTENT_EXT', '.md');
-
-require_once(VENDOR_DIR . 'autoload.php');
-require_once(LIB_DIR . 'pico.php');
-$pico = new Pico();
+require_once(__DIR__ . '/vendor/autoload.php');
+$pico = new Pico(
+ __DIR__,
+ 'config/',
@dav-m85
dav-m85 Oct 6, 2015 Contributor

I wonder if we could move configuration directly in index.php, instead of maintaining a separate configuration file (or at least code the configuration retrieval here).

Let me elaborate. I'm using Pico within a Silex installation, redirecting all calls to a specific pattern to the Pico->run function. Thing is Silex has already its configuration system, and having Pico duplicates a bit the logic (I need to maintain/deploy one conf file specifically for it).

Something cool would be:

// index.php
$confArray = array(
    'someconf'=>array()
);
$engine = new Pico($confArray);
$engine->run();

... but of course it would oblige us to remove index.php from the repo, provide template for it and make things more complicated in the end.

One way to mitigate this would be to change the Pico constructor signature, kinda "if $config is array then load it instead of fetching a configuration file".

Of course, this is a new feature and it would be totally ok to postpone it until 1.0 is merged. I can provide PR if the feature sounds usefull to you guys. @theshka @PhrozenByte ?

@PhrozenByte
PhrozenByte Oct 6, 2015 Collaborator

Providing an easy way to integrate Pico in other applications was the intention of removing the constants, so this definitely is a completely fair use case. It is not necessary to move the whole config handling to index.php, it's entirely sufficient to provide an API to do such things - so I've added a Pico::setConfig() method. See method docs and 1419cf1 for details.

PhrozenByte added some commits Oct 6, 2015
@PhrozenByte PhrozenByte Add Pico::setConfig() method
Thanks @dav-m85
1419cf1
@PhrozenByte PhrozenByte Update changelog.txt for 1419cf1 04a1c60
@PhrozenByte PhrozenByte Allow multiple calls to Pico::setConfig() b09433a
@PhrozenByte PhrozenByte Use PSR-0 autoload
Makes no big difference... Using PSR-4 breaks BC.
7c5f371
@PhrozenByte PhrozenByte Update Picos inline user docs
Adding a Blogging and URL Rewriting section, splitting the Plugins section into "for users" and "for devs", extend all sections and fix some typos
40dbd0e
@PhrozenByte
Collaborator

I've updated Picos inline docs, feedback is appreciated (click here to see the parsed version). As said, I'm no english native speaker, it would be great if someone reviews what I wrote. The inline docs should contain answers to any questions a typical user could have, so please check for completeness, too.

@theshka
Collaborator
theshka commented Oct 8, 2015

I can go through and review spelling/grammar @PhrozenByte... Would you like them as line comments or pull-request on your fork?

@PhrozenByte
Collaborator

A PR would be great, thanks @theshka

PhrozenByte and others added some commits Oct 27, 2015
@PhrozenByte PhrozenByte Improve inline code comments; preparing use of phpDocumentor 92af554
@PhrozenByte PhrozenByte phpDocumentor 2.8.5 currently doesn't support the Generic notations
This will likely be implemented as soon as the proposed PSR-5: PHPDoc is accepted
a654b15
@PhrozenByte PhrozenByte Fix Markdown %meta.*% replacement
Don't even try to use arrays here...
de6b3a7
@PhrozenByte PhrozenByte Prevent content_dir breakouts using malicious request URLs
It's appalling that nobody (including me!) thought about that!
9e2604a
@PhrozenByte PhrozenByte Trap empty $requestFileParts 647a7b5
@dav-m85 @PhrozenByte dav-m85 Create .travis.yml 3e0161b
@PhrozenByte PhrozenByte Update .travis.yml
Just adding some features inspired by other projects using Travis, e.g. a simple PHP syntax checker with various PHP versions. A short peak into @dav-m85 link leads me to think that running composer and creating the archive should be done with before_deploy rather than script.
d3a1308
@PhrozenByte PhrozenByte Update .travis.yml
Build on picocms/Pico only
efcbbb8
@PhrozenByte PhrozenByte Update changelog.txt
- Add security section
- Add Travis CI
a068a1f
@PhrozenByte PhrozenByte Update .travis.yml
Use $TRAVIS_TAG for the "binary" filename
360e7ab
@PhrozenByte PhrozenByte Update .travis.yml
Use Travis container-based infrastructure
43f9590
@theshka theshka referenced this pull request Oct 28, 2015
Closed

Pico v0.9 and beyond.... #221

@dav-m85
Contributor
dav-m85 commented Oct 28, 2015

👍 travis

@theshka theshka referenced this pull request Oct 28, 2015
Closed

Pico 1.0 development & release process #268

7 of 7 tasks complete
PhrozenByte added some commits Oct 28, 2015
@PhrozenByte PhrozenByte Update .travis.yml
Let Travis build branches master and gh-pages only
a068850
@PhrozenByte PhrozenByte Sync docs with website 38081b3
@PhrozenByte PhrozenByte Add UPGRADE section to docs
This is still work in progress
638638f
@PhrozenByte PhrozenByte Revert commit a068850
According to travis-ci/travis-ci#2111 and some own testing, it isn't possible to combine branch whitelists with tag-based auto deployment. Unfortunately it is necessary to whitelist the gh-pages branch, because Travis implicitly blacklists it.
f1fc4c9
@PhrozenByte PhrozenByte Various small improvements
- Improve class docs for phpDocumentor
- Add missing onPagesLoading() event to DummyPlugin
- Add some TODOs to the UPGRADE section of the docs
54ce5b9
@PhrozenByte PhrozenByte Add CONTRIBUTING.md
The file mostly contains adapted contents @theska wrote for our website and the great CONTRIBUTING.md of phpDocumentor (https://github.com/phpDocumentor/phpDocumentor2/blob/v2.8.5/CONTRIBUTING.md)
d29e2c1
@PhrozenByte PhrozenByte Update CONTRIBUTING.md 7a69fdf
@PhrozenByte PhrozenByte Improve class docs
Also add some ToDos to inline docs
e6681ea
@PhrozenByte PhrozenByte Improve class docs afb55b9
@PhrozenByte PhrozenByte Remove `return $config` in `config/config.php`
I always thought that doing this is pretty unusual... But now it simply breaks BC - please refer to @Lomanic's [comment](#260 (comment)). Using a return statement has no advantages, but increases the probability that something goes wrong (e.g. a clueless user removes the return statement). It was introduced with 23b90e2, but we never released it ([v0.9.1](https://github.com/picocms/Pico/blob/4cb2b24fae6bda83c7a3327cfb806370a90a60ac/lib/pico.php#L188-L189)). Removing the return statement shouldn't cause any problems even for users which installed Pico in the meantime. As a result we don't break BC and moreover remove a prior BC break 😃
9a70241
@PhrozenByte PhrozenByte PicoDeprecated: Making $config globally accessible again
This was dropped without a replacement with Pico 0.9. I checked all changes since Pico 0.8 manually, as far as I can tell there should be no more surprises regarding BC... Thanks @Lomanic for rubbing our nose in the fact that we should check this! I also added the missing changes of Pico 0.9 to changelog.txt
8da62f4
@PhrozenByte PhrozenByte Update CONTRIBUTING.md 85d7c51
@PhrozenByte
Collaborator

Heads up! I'll merge this PR on 6th November, the new release will be called 1.0.0-beta.1. I would like to call you up for testing the new release, feedback is always appreciated!

PhrozenByte and others added some commits Nov 2, 2015
@PhrozenByte PhrozenByte Move upgrade instructions from inline docs to README.md in a more gen…
…eric form
2597e0d
@PhrozenByte PhrozenByte Guess content directory
As pointed out by @Lomanic (see #260 (comment); thank you btw\!) we actually have to explain users how to change the content directory. This runs contrary to our "stupidly simple" claim. So Pico now simply uses the `content` directory when it exists...
ebe007b
@PhrozenByte PhrozenByte Update inline docs to reflect ebe007b a1331e7
@PhrozenByte PhrozenByte Update changelog.txt to reflect ebe007b 8db3bc5
@PhrozenByte PhrozenByte Update README.md
- New Contributing section
- Various small improvements
92adb27
@PhrozenByte PhrozenByte Update CONTRIBUTING.md
Add "Keep documentation in sync" section
0e0eb56
@PhrozenByte PhrozenByte Merge branch 'pico1.0' of github.com:PhrozenByte/Pico into pico1.0 641a5d1
@PhrozenByte PhrozenByte Update README.md d65eb55
@PhrozenByte PhrozenByte Update content-sample/index.md 2ab3611
@PhrozenByte PhrozenByte README.md: Add Getting Help section ffc5936
@PhrozenByte PhrozenByte Update content-sample/index.md 365333c
@PhrozenByte PhrozenByte Move license.txt and changelog.txt 48264fc
@PhrozenByte PhrozenByte Use Markdown for CHANGELOG.md 8164038
@PhrozenByte PhrozenByte Small changes
- Use Travis build status
- Update version number in config template
82e0ca5
@PhrozenByte PhrozenByte Fix possible foreach on null errors 90128f4
@PhrozenByte PhrozenByte Sync documentation e3a6116
@PhrozenByte PhrozenByte Force trailing slash of $config['base_url'] and force existance of $c…
…onfig['timezone']
ccac8dd
@PhrozenByte PhrozenByte Enable PicoDeprecated if no plugins are loaded 2a43b21
@PhrozenByte PhrozenByte Fix typos af8de56
@PhrozenByte PhrozenByte Sync documentation 19f708e
@PhrozenByte PhrozenByte Update .travis.yml: Name release archives "pico-release-$TRAVIS_TAG.t…
…ar.gz"

Should make it easier for a ordinary user to distinct source code and release 😃
b5d54d0
@PhrozenByte PhrozenByte Add content filter to get the parsed contents of a page (lazy loading) cd7cd37
@PhrozenByte PhrozenByte Fix typo e3e0300
@theshka theshka fix typos 82cbe37
@PhrozenByte PhrozenByte Sync docs cd1dc07
@PhrozenByte PhrozenByte merged commit e5b0ec6 into picocms:master Nov 6, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PhrozenByte PhrozenByte deleted the unknown repository branch Nov 6, 2015
@smcdougall
Collaborator

Although I like the idea of 404-ing access to files that you wouldn't want a nosy viewer poking around in, this makes it very inconvenient to include other files (images, downloads) in a page. I know you could still put them in a dedicated folder (a /images or /assets), but this would get really disorganized with a large number of pages. Also, the need to separate your images into another folder seems slightly counter-intuitive to the advantages of a flat file cms. If I wanted all my images in a special, obscure folder, I'd use WordPress (jk). Also, when I say files or images, I mean ones specific to an individual page, not site-wide logos / branding / etc.

Personally, I've removed "content" from the rewrite rule, keeping the other folders blocked. This isn't really an ideal solution, so much as a stop-gap method though. What are your / the project's thoughts on the matter? Should I move my extra files from the content folder, where I feel they belong (they are "content" after all), to another location? Do you plan to add better support for images / files later on? Right now it's kind of a pain to link to files anyway because of how rewriting of urls and hiding the content folder works.

Didn't mean to make this such a rant. Only wanted to voice my feedback / start a discussion. Maybe I should have opened this as an issue. =/

Collaborator

We recommend a separate assets folder as this is common sense. Additionally relative URLs don't work as you would expect it: the file sub/page.md is accessible through http://example.com/sub/page, but the image sub/image.png is accessible through http://example.com/content/sub/image.png - quite messy, see #262.

Obviously you can still remove the RewriteRule as you did, but we neither recommend nor support this. Especially we will never do this as a default, we already know access control plugins, which are completely useless if the raw content dir is accessible from outside.

Collaborator

Okay. I'll probably shift some of my assets into a separate folder when I update my sites to 1.0. In my case, there's nothing sensitive in the content folder. Folder indexes are disabled, so the only thing you could access would be the files I have linked to anyway.

I've been using absolute paths for my images/etc so far, but I would definitely love to see a way to process relative links if it ever became a targeted feature.

As far as your recommendation goes, I'd suggest maybe adding an example folder into the Pico release package and/or mention something in the documentation. I came across Issue #262 a few days ago, but you didn't really go into the specifics of "keeping things separate", so I thought I'd ask here. A section in the documentation about "Images and other files" might be a good idea for the future. 😃

Collaborator

Unfortunately it's virtually impossible for Pico to put assets into the content folder without allowing access to the raw contents by default. The only way to achieve this would be passing all assets through PHP, what is a very bad idea regarding the websites performance.

As always in computing there are some other options to achieve virtually the same, but they are simply out of Picos scope. You can e.g. use rewrite rules as described in #262 (they will bypass the 404-rules), but supporting this officially would increase support effort and doesn't work with alternative webservers like Nginx or Lighty - even though you can again achieve virtually the same with them, just with a completely different config. Sure, you can improve security by using rewrite conditions (RewriteCond) to only allow access to typical asset file extensions (.png, .zip etc.), but all that is pretty user-dependent. It's actually a matter of individual webserver config, not of Pico.

I'm not sure what you mean by "process relative links to assets". You can simply use %base_url%/assets/image.png resp. {{ base_url }}/assets/image.png or do I miss something? Apart from that I recommend you to use e.g. $config['site_logo'] = 'assets/logo.png'; in your config (please note the missing leading slash, this way it's more consistent to e.g. content_dir) and {{ base_url }}/{{ config.site_logo }} in your theme, how should a "techy" solution look like? My first thought would be something like {{ config.site_logo|asset }}, but that's imho less obvious and easy to understand. The only reason for the link filter is that we must check for URL rewriting, with Twig we can't simply replace the URLs as we are doing it with the %base_url% placeholder in Markdown files.

We'll add a sentence to the docs telling users that we recommend to use a separate assets folder. Thanks again for your feedback @smcdougall! 😃

Collaborator

What I meant by "process relative links to assets" was just being able to say "image.jpg" (relative to the markdown file) instead of writing the path from document root / base_url (which is an absolute path, not a relative one, even if it contains a variable).

That does sound like it would be quite the nightmare of webservers and rewrite rules though. I can see why it would be out of the scope of the project.

The "/content/site_logo.png" in my theme is merely a suggested location for a logo, but I'll change it to "assets/site_logo.png" in my next release.

It's gonna be a big update. I actually started working on some new features before I knew there was a new Pico version. I only noticed because the documentation had updated and it was throwing me for a loop.

I'd love to help out more with the project if I could. I don't know very much php though, but judging by my rewrite I am getting pretty good with Twig... 😕 😄

Collaborator

We're seeking for help to improve the docs and the Pico website in general 😃

Collaborator

Okay. Where do I sign up? lol.

Can you point me in the right direction? I'm embarrassingly less familiar with Git and GitHub than I'd like to be. Do I just fork and pull request like your readme suggests?

What in particular needs to be worked on? I see I've already forced you to create a "cookbook" of future documentation pages. 😛

Collaborator

Yep, just fork the project and create pull requests 😃

There are some things which need to be done before releasing the final 1.0.0, you can find a ToDo list here. Beyond that the user docs need a complete overhaul (see the _docs collection, it's Markdown), they are just a copy of the inline user docs at the moment, but should cover much more things in a more structured form. The CSS of the website is in some parts not optimal (like formatting code) and looks a bit outdated, unfortunately my CSS skills aren't the best. As explained previously, the website is realized with Jekyll and its template engine Liquid (be warned: it's not a joy to work with them...).

@theshka theshka referenced this pull request Nov 16, 2015
Merged

Restructured upgrade.md #276

@PhrozenByte PhrozenByte removed the pri: High label Dec 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment