Split Source into multiple files during development and merge with Build script
Issue #190
resolved
Originally reported on Google Code with ID 190
Currently we develop PLA in one file. The one-file approach is good for the release
(install, deployment...), but bad for development. It can lead to lots of unnecessary
merging. And by splitting the code into several files, it would be easier to find the
relevant parts in the code etc.
So I would propose something like this for a start:
- phpliteadmin.php: Everything currently in there, except:
- defaulttheme.css: Default JS theme
- script.js: JavaScript. Possibly splitted into multiple files.
- favicon.ico: The favicon
- One file per (PHP) Class
This should be good start. the goal should be to move more and more code out of the
phpliteadmin.php into Classes.
Then we need a build-script that automatically bundles everything into one phplteadmin.php
file again that we can release.
This build script would more or less only paste the individual file data inside the
phpliteadmin.php. Maybe do some compression of JS and CSS.
During development, it is important that PLA runs without the need to run the build
script every time.
So basically, I think in phpliteadmin.php, we move some stuff out and replace it with
include()s. The build-script then only replaces the include()s by the actual file content.
Reported by crazy4chrissi on 2013-03-09 00:38:48
Comments (59)
-
reporter
-
Account Deleted
if i do it, i would do something like this (on image). templates would be in ob_start or somewhere at the end of file. So, everything will be in classes, while start up will look like Factory::getApp()->init(debug)->dispatch(); or Factory::getApp()->init(debug)->route()->dispatch();Reported by
ykorotiaon 2013-03-09 18:17:13<hr> * Attachment: schema.jpg
-
reporter
This issue is not about improving the architecture of PLA, it is about splitting files to simplify development. Or to rephrase it: It is not about restructuring or changing any code. It is about splitting the code that is already existing. Improving the development infrastructure, not PLA itself. Restructuring, like introducing an MVC architecture, as implied by your draft, is another issue. Although this could largely benefit from this issue (being infrastructure that supports restructuring).Reported by
crazy4chrission 2013-03-09 18:25:35 -
reporter
SQLite itself also is developed in lots of files (of course ;-)) - see https://www.sqlite.org/cgi/src/dir?ci=e899b058a7031580&name=src ) and then the make script builds one big file out of it. See https://www.sqlite.org/amalgamation.html .Reported by
crazy4chrission 2013-03-09 18:40:39 -
Account Deleted
I see. But you already do it with version 2, right? Or version 2 is dead? for better merging you have to remove duplicates of code and remove global variables, otherwise, it will be hard to get what variable is set and where. it is not mvc, your structure is simple, so I suggest to extend your app with configuration and registry classes. factory is also good. also many methods have many if-else, it is very hard to read (for me), but if it is good for you, nevermind then. Still I'm somewhere on line 900.. so don't get me too seriousReported by
ykorotiaon 2013-03-09 18:42:56 -
Account Deleted
about merge script, it can be like this MERGE.bat copy test1.txt+test2.txt test3.txtReported by
ykorotiaon 2013-03-09 18:48:38 -
reporter
> Still I'm somewhere on line 900.. so don't get me too serious lol. I have never read the code like this. > I see. But you already do it with version 2, right? Or version 2 is dead? This was planned for version 2, but nobody currently develops version 2, so I think it won't every come to live. Instead we incrementally improve the current version until we have what was planned for version 2. One of these steps is to split files. For version 2, the build-process was not implemented yet, so we cannot use anything from there. > for better merging you have to remove duplicates of code and remove global > variables, otherwise, it will be hard to get what variable is set and where. I am not sure what you mean. I plan to only move classes out of the phpliteadmin-file. So there should be almost no global stuff used in there.Reported by
crazy4chrission 2013-03-09 18:51:08 -
reporter
> MERGE.bat > copy test1.txt+test2.txt test3.txt No, that is not what I have in mind. The problem with this would be that you could not run PLA without running merge.bat after every change. This would make development harder (more annoying / requiring better IDEs). Instead, look at this example: phpliteadmin.php: <?php ... #INCLUDE include('classes/Authorisation.class.php'); .. ?> So we can run phpliteadmin.php during development without the need to run the build-script. Then we have a buildscript that replaces the include() marked by the #INCLUDE marker with the file referred to. I think for portability, we should best implement this in php. A windows batch file or linux make file does not run on both platforms, so we would need to write and maintain 2 of them. I thought of something like: build.php <?php $source=file_get_contents('phpliteadmin.php'); preg_match_all('/#INCLUDE\n\s*include\(\'([^\']+)\'\);/',$source,$matches); foreach($matches as $match) { $include_source = file_get_contents($match[1]); $source = str_replace($match,$include_source,$source); } file_put_contents('built/phpliteadmin.php'); ?> (just a draft to illustrate what I mean, not tested etc)Reported by
crazy4chrission 2013-03-09 19:06:32 -
Account Deleted
bat is enough to make bulk file, you can add/update bulk file in svn. For developer it is enough to have entry point like index.php which won't be included by bat file. about linux, it is simple as is $cat test1.txt test2.txt >> test3.txt i guess, you will have to have merged file in svn too. or you don't want to keep this file in repository? you can merge with php, the main question is - who will merge files?Reported by
ykorotiaon 2013-03-09 19:37:28 -
reporter
Hmm. Well, yes, we could have an index.php that includes all other files to run PLA during development like this: index.php <?php include('file1.php'); include('file2.css'); include('file3.js'); include('file4.php'); ... ?> We would need to make sure links work (i.e. point to the correct php-file) then, but this would be easy. And we could have a batch script for linux and windows to concat these files like your examples. But: We specify the list of files to include in 3 places then: the index.php, the windows build script and the linux build script. This can get inconsistent easily. That's comparably easy to fix, all 3 scripts could simply use all files form a certain directory automatically. But then, in which order? We would need to enumerate files. And then, what if we put a new file in between? rename all following files? Another way would be to have another central config-file for all 3 (build) scripts (like filelist.txt) that lists the files in the order to include them. But I think the solution of having normal php includes that get replaced by a build script still is a lot nicer and easier to figure out. > i guess, you will have to have merged file in svn too. or you don't want to keep > this file in repository? Good question. I guess we should not include it in the repository. The merged file is for the release. It would change with every commit or would be inconsistent with the other files. I think the best approach would be to setup a post commit web hook (which is supported by google code) that automatically runs the build script after every commit and makes the built file available on some webserver. This would allow us to always have a 1-file development version of PLA available. for example, I like to post links of the current development version to users so they can check if the bugfix works for them. It would not be so easy for them to checkout the whole source with multiple files.Reported by
crazy4chrission 2013-03-09 20:36:00 -
reporter
By the way: As I mentioned, minifying files might be an additional task that could be performed by the build script. So compressing js/css files, maybe even the php-file itself. Would be better to have a central automatically running build script that does this than to have linux and windows batch scripts that do this. Minify is a cool php script for this for example ( https://code.google.com/p/minify/ ),Reported by
crazy4chrission 2013-03-09 20:50:25 -
Account Deleted
what about next aproach: index.php?merge index.php <?php $files = array('file1.php', 'file2.php',...); if(isset('merge')) { $buf =''; foreach($files as $f) { $buf .= file_get_content($f); } file_put_content('pla.php', $buf); echo 'merged successfully'; die; } foreach ($files as $f) { include $f; }Reported by
ykorotiaon 2013-03-09 21:58:46 -
reporter
Hmm. That solves the problem of portability (by using php) and consistency (by using a single file-list). I think that approach is okay. But I still see a disadvantage. Example: file1.php <?php //part1 //part2 //part3 ?> Now you want to move part2 into a separate file. Then you need to create 3 files, ripping apart part1 and part3, although they might belong together: file11.php <?php // part1 ?> file12.php <?php // part2 ?> file13.php <?php // part3 ?> With my approach, you could insert an include between part1 and part3 so they stay together. Any other opinions?Reported by
crazy4chrission 2013-03-09 22:35:26 - Status changed:Accepted -
So much to read suddenly :) A couple of sparse comments before I get to the point: - we have php, let's avoid shell or batch scripting - rather than include, this is a job for 'require' - include/require are not functions, no need for brackets That said, I wouldn't use either of them (kinda). It's 2013 and we've had class autoloading for some time, spl_autoload¹ starting from php 5.1.2, and PSR² stuff is looming. I'd like to split classes into separate files and autoload them (and yes, that requires 'require's) and for release we simply concatenate the main file with the class files, without changing a line of code. The build script can be php and handle resource inclusion, minifying and encoding —so we can use php-minify instead of YUI Compressor. The split version and bundled version should live in parallel, to make switching from one to the other (that is, development) easier. I'd suggest this directory structure /-- (version directory) | +-- build-pla.php +-- index.php ( main pla script, non-oo code ) +-- phpliteadmin.config.php ( personal, works for both versions, not in repo ) +-- phpliteadmin.config.sample.php +-- phpliteadmin.php ( built by script ) +-- resources/* ( css/js/etc ) `-- classes/* ( ClassName.php ) The easiest way to embedding resources would be to have some placeholders inside the code, but maybe we can work around that with a smarter Resources class. [1] http://www.php.net/spl_autoload [2] http://www.php-fig.org/Reported by
dreadnauton 2013-03-10 11:21:27 -
I had a look at php minify, the script linked above, and I'm a bit uncertain. Of the whole package, we need only two classes (Minify_JS_ClosureCompiler and Minify_CSS_Compressor), but the first contacts an online service to compile javascript and will not work without an internet connection. I'll look around, hopefully there's a similar php solution with a self-contained implementation.Reported by
dreadnauton 2013-03-13 16:00:25 -
reporter
Reported by
crazy4chrission 2013-03-14 00:49:00 - Labels added: Target-1.9.5 -
I was just trying the split + autoload system now and a 5-lines function is enough to hold everything together. If you like the method I can prepare a patch and commit it first thing after we release 1.9.4, so we can then work on the build script in parallel with other development.Reported by
dreadnauton 2013-03-14 00:52:51 -
reporter
Yes, autoload definitely seems to be the correct way to load classes, better than the other 2 approaches. I think this should be the first step, so yes, please prepare a patch. We then need to make sure it is easy to edit css & js while developing (for css it is already easy as we can set $themes to the css file and that's it. But for JS we need to do something similar.) > I had a look at php minify, the script linked above, and I'm a bit uncertain. Well. In my opinion, we could simply setup the build script on some webserver so it always automatically builds the current development version. Then we could always have a built 1-file-version available. Nobody would need to setup the build script anywhere else. Then it would be no problem if we only need a small portion of minify or if it requires an internet connection. But this also means we could use YUI compressor for example. I am not sure if minify uses this as well, but Google also offers a JS minify API: https://developers.google.com/closure/compiler/ But if you find some great php class that does our job so we can create a small php build script that anybody can setup easily, of course this would be a lot better. When we are done with this issue, we should create a wiki page that explains some things on how to develop PLA most easily.Reported by
crazy4chrission 2013-03-14 11:51:09 -
reporter
Ah. On another project, I was using these 2: http://code.google.com/p/cssmin/ https://github.com/rgrove/jsmin-php/ The later one is unmaintained and says "You shouldn't use it." ;-) (Nevertheless it worked great when I used it...) It recommends this one (and google closure I mentioned above): https://github.com/mishoo/UglifyJS2 But this is not php, it is js (node.js).Reported by
crazy4chrission 2013-03-14 11:57:34 -
reporter
Here are some more to check out: https://github.com/tedivm/JShrink (looks good at first sight) http://crisp.tweakblogs.net/blog/cat/716Reported by
crazy4chrission 2013-03-14 12:05:13 -
There a JSMin+ but I'm not sure of its status. I'll look around, but if we don't mind java the YUI Compressor might be one of the best solutions. About autoloading, this is what I tried: @@ -361,6 +361,20 @@ define("FORCETYPE", false); //force the extension that will be used (set to false in almost all circumstances except debugging) define("SYSTEMPASSWORD", $password); // Makes things easier. +// setup class autoloading +function pla_autoload($classname) +{ + $classfile = __DIR__ . '/classes/' . $classname . '.php'; + + if (is_readable($classfile)) { + include $classfile; + return true; + } + return false; +} + +spl_autoload_register('pla_autoload'); + // Resource output (css and javascript files) // we get out of the main code as soon as possible, without inizializing the session if (isset($_GET['resource'])) {Reported by
dreadnauton 2013-03-14 12:07:37 -
reporter
Yes, looks good. But didn't you say yourself that this is a job for require? ;-)Reported by
crazy4chrission 2013-03-14 12:14:35 -
Not in an autoloader :-p If you can't find or load the file you should not halt the script, but return false and let the next autoloader in the chain try.Reported by
dreadnauton 2013-03-14 12:22:01 -
reporter
Okay, true. But as long as there is only one autoloader, it doesn't really make a difference, right? "Class not found" is a fatal error, so PHP will halt anyway if a class is missing (once used, and autoloaders don't get called if the class is not used).Reported by
crazy4chrission 2013-03-14 12:28:39 -
| But as long as there is only one autoloader, it doesn't really make a difference, right? Correct, it would be the same *if* there were only one autoloader. If someone integrates PLA with their software there might be more than one, so we shouldn't break the autoloading chain for no good reason. Attached is a sample split version so we have somewhere to start from. It uses Minify's CSS compressor class and JShrink for javascript (both stored inside the support/ directory). index.php contains the main code, while each class has its own source file under classes/. The build script concatenates the main index.php with classes/*.php, removing the first php opening tag. Resources are then prepared and replace any occurrences of :::filename::: in the source code. I modified the Resources class accordingly to work in split mode: if the resource data contains :::filename::: it will load the external file.Reported by
dreadnauton 2013-03-14 14:54:19 - Status changed:Started<hr> * Attachment: 1.9.5-test.zip
-
Reported by
dreadnauton 2013-03-18 16:29:30 - Blocking:#192 -
New version, based on the code released for 1.9.4. Same changes as above: split classes, added autoloading (about line 390), added a basic build script. Should we place languages in a subdirectory? The root directory might get messy as we get more locale files, but we might also have to change some code to accomodate that. I think themes now do something similar, searching first in the current directory and the under themes/.Reported by
dreadnauton 2013-03-18 18:25:04<hr> * Attachment: 1.9.5-test.zip
-
reporter
Languages also get loaded from "languages". No problem to move them there. Works already. We should think about how we deal with the English stuff. Because I think it would be good to move this outside of the main file as well. (I have not had a look at your upload yet, will do so in a minute)Reported by
crazy4chrission 2013-03-18 18:28:44 -
reporter
Looks like a very good start. I really like the Resource handling. It introduces 3 LOC that are useless in the build version, but that's clearly worth it. I would do the replacement of resources only on the code of the Resource class. Otherwise we introduce assumptions like "you must not have :::resources/phpliteadmin.css::: anywhere in your code". Although this assumption is quite safe, I would prefer to restrict it to the Resource class. About the <?php removal: I was thinking about whether it would be better to simply add ?> at the end of every php file. Then concatenation would work without removing "<?php". On the other hand, some useless "?>\n<?php" will end up in the code. Maybe your solution is better. As said before, I would prefer to move the English language definition out of index.php as well. At the moment we have it in 2 places, which can lead to redundancy problems. We could either implement something special in the Build-Script or change the language system somehow so it is a class that gets autoloaded to achieve that. When I look at what remains in index.php, this looks like quite a lot of work until we get this "nice". We should remove or adjust the #eof comment because with the build script it ends up in the middle of the file ;-)Reported by
crazy4chrission 2013-03-18 18:54:46 -
Closing php tags are a risk, you might end up sending a couple of newlines to output. With autoloading, files are included on the fly when you try to instantiate a class, which means that those newlines might end up in output before headers. Bugs waiting to happen IMO :) Saying "the first line of a class file must be <?php" is a tame requirement in the end. The same for "class files should produce no output on loading." I'm uncertain about the Resource implementation, and I'll see if I can do better using __halt_compiler()¹. The current code mixes code and data too much, and would not scale well. That would also solve the :::something::: replacement in other parts of the code. English strings: good idea, although we can do that later. Let's split the source as soon as possible, as it will make working on multiple changes easier. | We should remove or adjust the #eof comment because with the build script it ends | up in the middle of the file ;-) Ah, yes! Maybe replace it with #main-code-ends-here, or just drop it. [1] http://php.net/manual/en/function.halt-compiler.phpReported by
dreadnauton 2013-03-18 23:18:16 -
I'm on it anyway, taking ownership :)Reported by
dreadnauton 2013-03-19 00:37:31 -
reporter
| Closing php tags are a risk, you might end up sending a couple of newlines to output. Ah, right. Although you can end a file without a newline (and thus concatenate files without introducing one), some editors tend to add newlines at the eof, so your solution really seems to be safer. | I'll see if I can do better using __halt_compiler() I did not know this one. In the end it is only an "exit;" with the advantage that you can easily jump to __COMPILER_HALT_OFFSET__ and don't have to determine the position yourself. But looks very suitable in our case. | English strings: good idea, although we can do that later. Okay. | Maybe replace it with #main-code-ends-here, or just drop it. I think a comment like this might be useful (e.g. to search for the position). | I'm on it anyway, taking ownership :) Thanks.Reported by
crazy4chrission 2013-03-19 17:07:55 -
Ok, here's a different take on resources AND building. As I wrote above I am switching to __halt_compiler(), not only works as 'exit', but because whatever follows it is *completely ignored*: not parsed, not sent to output, nothing. You can have megabytes of binary madness after the call, and they won't affect the execution. This however makes the build script a bit more complicated, so I've added an external "build template" which contains directives to include the right files in the right position. Check 'build.php' for more details, I spent some time document the script so it's not black magic. A few changes to the Resources class as well, which now calls getInternalResource() to load data from the script file itself. It's not the *cleanest* implementation, but I think better than the previous version.Reported by
dreadnauton 2013-03-19 19:23:44<hr> * Attachment: 1.9.5-test-b.zip
-
Sorry, there was a duplicated line in classes/Resources.php (line 53) which broke split-file execution. Attached is the correct file.Reported by
dreadnauton 2013-03-20 14:28:35<hr> * Attachment: Resources.php
-
reporter
Just had a look t your new solution "b". Looks very cool. I consider this more or less an advanced version of what I had initially in mind. Very flexible. What I noticed is that in the build script, you always work on $output. This means in case any of the files you put in $output contains some build-command interpreted next, it will get replaced. This means for example that #EMBED commands could also be placed inside of index.php or classes (but not #INCLDUE commands). I am still thinking about whether this can be useful or if it is rather dangerous. (As resources get base64 encoded, no build-command can accidentally get introduced in there as they require "# " which cannot be base64. ) If we could also put #INCLUDE commands in the index.php, this would mean we could include lang_en.php this way. But then we could also put everything in index.php and wouldn't need the template... But I mean in the end, this is also a suitable solution that we could use and improve over time.Reported by
crazy4chrission 2013-03-20 17:26:57 -
Great feedback, thanks Chris. I am indeed following your first post :) Regarding working on $output, that's a good point, I missed it. We can work around it by first processing EMBEDs first and INCLUDEs second, though. I'd rather not have #EMBED commands appearing in other files: code in one place, data in another. A first version of the build script didn't use an external template and the directives were all at the bottom of index.php. It felt a bit dirty, so I switched to a separate file. We could #INCLUDE the english translation as default. At that point the external template cannot be used though, since the lang data must appear toward the top of the file —unless we also separate the license text and the default config (oh, wait we already do the latter!) I see two alternatives then: a) everything in index.php b) split more! In (b) we end up with a template more or less like this: <?php #INCLUDE docs/header.txt | comment_the_whole_thing #INCLUDE phpliteadmin.config.sample.php #INCLUDE languages/lang_en.php #INCLUDE index.php // resource embedding code function getInternalResource($res) { ... } #EMBED resources/* [...] I like including the default config, although this brings back some of the code and comments we removed when we added the sample config.Reported by
dreadnauton 2013-03-20 21:17:44 -
reporter
I also like b) More splitting is fine as long as we don't introduce too much coupling of different files. Splitting everything out of the main file that is "a thing of its own" is good. And default-config and the header comments can really be considered a "thing of its own". Aside: Thinking about the header: We could replace the manually updated "last updated" comment in the header with a date inserted by the build-script. #INCLUDE docs/header.txt | insert_build_date | comment_the_whole_thing > I like including the default config, although this brings back some of the code > and comments we removed when we added the sample config. Well, the very good point of it is that we reduce redundancy. If we add another config variable, adding it to one file would be enough (at the moment, we'd need to add it in 2 files). About the comments and code: Well, I don't mind much about this. But if we want to, we can also make our build script smart enough to remove some lines (marked somehow?). #INCLUDE languages/lang_en.php | remove_marked_linesReported by
crazy4chrission 2013-03-20 22:06:55 -
reporter
Last line was meant to be: #INCLUDE phpliteadmin.config.sample.php | remove_marked_linesReported by
crazy4chrission 2013-03-20 22:08:57 -
Currently, the build script remove all single-line comments starting with # (rather than //), even if they are not directives. I used it to add "build comments" here and there but is not essential to my implementation. We can make it a convention and use it to remove lines. Otherwise, we can strip lines between to special comments, e.g.: # IGNORE function should_not_be_in_the_single_file_code() { ... } # END IGNORE Last updated: I'd like to automate that and add it only to release and RC versions. I always forget to update it, and it will be even more difficult after the split, since we would have to commit at least two files every time!Reported by
dreadnauton 2013-03-20 22:55:54 -
I've had a look at implementing comment #37 and building works alright. However, we need to include the default configuration otherwise for the split-file version to run :| Something like this would work, but maybe there's a nicer way. # IGNORE include 'phpliteadmin.config.sample.php'; # END IGNORE Also, following Issue #197 and Lonnie's email* pointing out that we cannot give zlib for granted, I'm going to remove gzencoding from the build script. [*] https://groups.google.com/d/msg/phpliteadmin/gipdRoGVkjo/VIntw7P7cF0JReported by
dreadnauton 2013-03-21 15:38:43 -
Sorry, link to the wrong message, here's the correct one: https://groups.google.com/d/msg/phpliteadmin/gipdRoGVkjo/H8WevWOmUqkJReported by
dreadnauton 2013-03-21 15:39:41 -
reporter
> Something like this would work, but maybe there's a nicer way. Hmm. We could do something like this, but I think it's not better than your solution: if(!isset($databases)) // can only happen in split-mode, as the built version defines it include 'phpliteadmin.config.sample.php'; But the #IGNORE build-command could be useful anyway I think. > we cannot give zlib for granted, I'm going to remove gzencoding from the build script Yes. Zlib is always built-in on Windows, but not on Linux where you need to compile php with zlib and this is not the default. See http://www.php.net/manual/en/zlib.installation.php So we should not rely on it. If we use it, we first need to make sure it's available. But as the use is not important in this case, we probably better shouldn't use it at all.Reported by
crazy4chrission 2013-03-21 15:50:29 -
I'll go for an "#IGNORE" syntax then. Although... do you have any suggestions for a better keyword? #SPLIT-ONLY, #DON'T-BUILD, #BUILD:NO, #DEVEL —I can't find a good one :| For reference: we can use extension_loaded('zlib') to check for support. http://www.php.net/extension_loadedReported by
dreadnauton 2013-03-21 15:59:46 -
reporter
Hmm. This command will make it not only in the template, but in the index.php. So some guys hacking pla might see it and we should give it a name so they know what's going on. My suggestion: # REMOVE-FROM-BUILD ... # END REMOVE-FROM-BUILDReported by
crazy4chrission 2013-03-21 16:36:55 -
Ooook, maybe we are getting there. Here's version -C-. - added # REMOVE_FROM_BUILD ... # END REMOVE_FROM_BUILD build directive to ignore lines, available both in the build template and in included files; - added ###identifier### build directive to be replace with the content of a variable, available in all files. The former is used to include the default files in split-mode, and the latter is used to insert $build_date in header.txt. Maybe ###[\w+]### is not "safe" enough? - new build template that includes docs/header.txt, the sample config file and the English strings; - removed zlib compression, resources are now only minified; - swapped EMBED and INCLUDE to avoid embedding resources in included files; EMBED should now work only in the build template; - added comment_lines filter to comment header.txt.Reported by
dreadnauton 2013-03-21 19:15:46<hr> * Attachment: 1.9.5-test-c.zip
-
reporter
Thanks! > added # REMOVE_FROM_BUILD ... # END REMOVE_FROM_BUILD build directive to ignore > lines, available both in the build template and in included files; Hmm. Does this make any sense in the build template? It will only be used when building. Having something in there that is removed while building is a bit weird. Well, it's some kind of multiline-comment, okay. (I know, it is easier to do it this way, and as it doesn't introduce any problems, it's perfectly fine.) > Maybe ###[\w+]### is not "safe" enough? Hmm. If the build-script acted as documented in the comments, I'd say it is safe enough: // ###identifier### // Is replaced with the value of the variable $identifier, if defined "if defined" is not what the script does. If I append ###bla### to header.txt, it will get removed in phpliteadmin.php (i.e. replaced by nothing). Instead the build script should look up if the variable is set and skip ###bla### because $bla is undefined. If it acted like this, I would consider it "safe enough" because only very few ###identifier### are valid. To improve this, I would explicitly introduce an array of stuff that can be an identifier like $identifiers['build_date']=date('Y-m-d'); This would make sure only what is clearly defined as an identifer will get replaced. Otherwise, this could cause serious trouble: // This is where the ###output### of the script starts But we don't have to make this perfect now. We can start using it and see how good it works and improve it over time.Reported by
crazy4chrission 2013-03-21 20:58:40 -
| "if defined" is not what the script does. If I append ###bla### to header.txt, | it will get removed in phpliteadmin.php (i.e. replaced by nothing) Ah, I knew I had forgotten something. Fixed it: if the variable is not defined, the string will not be replaced. Also, I added a $build_data array (similar to $identifiers as you suggested) which works for both ###something### and #VAR, which I have renamed as #EXPORT, though. I also reshaped the build script comments into some kind of documentation, and added a few checks and warning when unexpected stuff happens (missing files, undefined variables). I fairly happy with this version, so if no-one has further comments on it I'm ready to commit it under 1.9.5.Reported by
dreadnauton 2013-03-22 15:02:54<hr> * Attachment: 1.9.5-test-d.zip
-
reporter
Thanks a lot for your great work! Looks very good. I think we can start using it, so please commit it in 1.9.5. But please have a look at the commits that have been made to the 1.9.5 branch and make sure they won't get reverted.Reported by
crazy4chrission 2013-03-23 13:05:59 -
Here we go: rebased on the latest 1.9.5 and committed as r385.Reported by
dreadnauton 2013-03-24 16:35:30 - Status changed:Fixed -
Of course I made a mix-up while diffing my index.php with the code from 1.9.5. with r386 I noticed a first sign, and r387 fixes the last issues —mainly, I forgot to remove some of the code split to external files.Reported by
dreadnauton 2013-03-24 17:23:21 -
reporter
Thanks a lot! I just had another idea: With this build-system that works much like conditional compilation in C (#IFDEF), we could offer different builds of PLA like one with support for SQLitev2 and one only for v3. I think there should be a bunch of code in the DB-class (and, unfortunately, not only there :( ) that is only a sqlite2 workaround. But let's forget this, we have a lot more important things to do. Saving 10 KB of size for some users probably will never be worth the effort of releasing and maintaining multiple versions.Reported by
crazy4chrission 2013-03-25 09:24:59 -
reporter
I noticed a problem: Currently, custom functions are defined both in phpliteadmin.config.sample.php and in (built) phpliteadmin.php. Thus, renaming phpliteadmin.config.sample.php to phpliteadmin.config.php yields this error: Fatal error: Cannot redeclare md5rev() ... I see two solutions: 1. insert # REMOVE_FROM_BUILD into phpliteadmin.config.sample.php + This will make phpliteadmin.php a bit smaller. - But users might be confused by the comments (but removing them does not break anything as long as they don't build pla). 2. function_exists() around all custom function declarations. + Users understand function_exists() -- the function in phpliteadmin.php gets used because it is defined earlier. Users will be confused that changing phpliteadmin.config.php does have no effect. So I guess we need to go for 1.Reported by
crazy4chrission 2013-03-25 23:24:05 -
reporter
ah, 1 more disadvantage of 1.) :-( Does not work the same way during development. index.php will first include the sample-config and then the user config. Build command has no effect here. This means developers must remove custom functions from their user config.Reported by
crazy4chrission 2013-03-25 23:27:45 -
I would leave only one sample custom function, maybe a more interesting one (capitalize? leet_text?), and then add a single call to function_exist(). We can improve the comment if it looks unclear, but it's an advanced feature that php-ignorant users will probably ignore anyway. //a list of custom functions that can be applied to columns in the databases //make sure to define every function below if it is not a core PHP function $custom_functions = array('md5', 'sha1', 'time', 'strtotime', 'leet_text'); //define all the non-core custom functions if (!function_exists('leet_text')) { function leet_text($value) { return str_replace(...); } }Reported by
dreadnauton 2013-03-26 19:17:04 -
reporter
Hmm. But still, I think the function_exists() approach has a real problem: It will use the function defined in phpliteadmin.php. So if somebody changes the function in phpliteadmin.config.php, his changes will have no effect. I have another idea: We can just out-comment the function. People who are capable of writing a php-function will know what to do ;-) //define all the non-core custom functions /* This is an example function. To use it, remove the comments around it. function leet_text($value) { return str_replace(...); } */ But one small problem then would remains: Even lots of users might not add custom functions, some might use the example functions that are kind of built-in. Okay. Looking at them, probably nobody uses them :-DReported by
crazy4chrission 2013-03-26 19:25:55 -
Ah, comments! The simplest solution is actually the best :) We should also comment the identifier in the array above //a list of custom functions that can be applied to columns in the databases //make sure to define every function below if it is not a core PHP function $custom_functions = array( 'md5', 'sha1', 'time', 'strtotime', // add the names of your custom functions here! /* 'leet_text', */ ); // define your custom functions here! /* function leet_text($value) { return str_replace(...); } */Reported by
dreadnauton 2013-03-27 01:24:42 -
Here's a patch equivalent to my previous post.Reported by
dreadnauton 2013-03-29 00:34:11<hr> * Attachment: custom.diff
-
reporter
Looks good. Commit it.Reported by
crazy4chrission 2013-04-02 22:40:45 -
Done, now in the repository as r396.Reported by
dreadnauton 2013-04-03 23:51:47 - Log in to comment
Reported by
crazy4chrission 2013-03-09 00:39:35