Closed GoogleCodeExporter closed 9 years ago
(this was mentioned as a comment in issue #157).
Original comment by crazy4ch...@gmail.com
on 9 Mar 2013 at 12:39
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();
Original comment by ykoro...@gmail.com
on 9 Mar 2013 at 6:17
Attachments:
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).
Original comment by crazy4ch...@gmail.com
on 9 Mar 2013 at 6:25
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 .
Original comment by crazy4ch...@gmail.com
on 9 Mar 2013 at 6:40
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 serious
Original comment by ykoro...@gmail.com
on 9 Mar 2013 at 6:42
about merge script, it can be like this
MERGE.bat
copy test1.txt+test2.txt test3.txt
Original comment by ykoro...@gmail.com
on 9 Mar 2013 at 6:48
> 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.
Original comment by crazy4ch...@gmail.com
on 9 Mar 2013 at 6:51
> 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)
Original comment by crazy4ch...@gmail.com
on 9 Mar 2013 at 7:06
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?
Original comment by ykoro...@gmail.com
on 9 Mar 2013 at 7:37
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.
Original comment by crazy4ch...@gmail.com
on 9 Mar 2013 at 8:36
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/ ),
Original comment by crazy4ch...@gmail.com
on 9 Mar 2013 at 8:50
[deleted comment]
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; }
Original comment by ykoro...@gmail.com
on 9 Mar 2013 at 9:58
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?
Original comment by crazy4ch...@gmail.com
on 9 Mar 2013 at 10:35
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/
Original comment by dreadnaut
on 10 Mar 2013 at 11:21
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.
Original comment by dreadnaut
on 13 Mar 2013 at 4:00
Original comment by crazy4ch...@gmail.com
on 14 Mar 2013 at 12:49
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.
Original comment by dreadnaut
on 14 Mar 2013 at 12:52
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.
Original comment by crazy4ch...@gmail.com
on 14 Mar 2013 at 11:51
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).
Original comment by crazy4ch...@gmail.com
on 14 Mar 2013 at 11:57
Here are some more to check out:
https://github.com/tedivm/JShrink (looks good at first sight)
http://crisp.tweakblogs.net/blog/cat/716
Original comment by crazy4ch...@gmail.com
on 14 Mar 2013 at 12:05
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'])) {
Original comment by dreadnaut
on 14 Mar 2013 at 12:07
Yes, looks good. But didn't you say yourself that this is a job for require? ;-)
Original comment by crazy4ch...@gmail.com
on 14 Mar 2013 at 12:14
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.
Original comment by dreadnaut
on 14 Mar 2013 at 12:22
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).
Original comment by crazy4ch...@gmail.com
on 14 Mar 2013 at 12:28
| 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.
Original comment by dreadnaut
on 14 Mar 2013 at 2:54
Attachments:
Original comment by dreadnaut
on 18 Mar 2013 at 4:29
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/.
Original comment by dreadnaut
on 18 Mar 2013 at 6:25
Attachments:
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)
Original comment by crazy4ch...@gmail.com
on 18 Mar 2013 at 6:28
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 ;-)
Original comment by crazy4ch...@gmail.com
on 18 Mar 2013 at 6:54
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.php
Original comment by dreadnaut
on 18 Mar 2013 at 11:18
I'm on it anyway, taking ownership :)
Original comment by dreadnaut
on 19 Mar 2013 at 12:37
| 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.
Original comment by crazy4ch...@gmail.com
on 19 Mar 2013 at 5:07
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.
Original comment by dreadnaut
on 19 Mar 2013 at 7:23
Attachments:
Sorry, there was a duplicated line in classes/Resources.php (line 53) which
broke split-file execution. Attached is the correct file.
Original comment by dreadnaut
on 20 Mar 2013 at 2:28
Attachments:
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.
Original comment by crazy4ch...@gmail.com
on 20 Mar 2013 at 5:26
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.
Original comment by dreadnaut
on 20 Mar 2013 at 9:17
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_lines
Original comment by crazy4ch...@gmail.com
on 20 Mar 2013 at 10:06
Last line was meant to be:
#INCLUDE phpliteadmin.config.sample.php | remove_marked_lines
Original comment by crazy4ch...@gmail.com
on 20 Mar 2013 at 10:08
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!
Original comment by dreadnaut
on 20 Mar 2013 at 10:55
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/VIntw7P7cF0J
Original comment by dreadnaut
on 21 Mar 2013 at 3:38
Sorry, link to the wrong message, here's the correct one:
https://groups.google.com/d/msg/phpliteadmin/gipdRoGVkjo/H8WevWOmUqkJ
Original comment by dreadnaut
on 21 Mar 2013 at 3:39
> 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.
Original comment by crazy4ch...@gmail.com
on 21 Mar 2013 at 3:50
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_loaded
Original comment by dreadnaut
on 21 Mar 2013 at 3:59
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-BUILD
Original comment by crazy4ch...@gmail.com
on 21 Mar 2013 at 4:36
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.
Original comment by dreadnaut
on 21 Mar 2013 at 7:15
Attachments:
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.
Original comment by crazy4ch...@gmail.com
on 21 Mar 2013 at 8:58
| "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.
Original comment by dreadnaut
on 22 Mar 2013 at 3:02
Attachments:
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.
Original comment by crazy4ch...@gmail.com
on 23 Mar 2013 at 1:05
Here we go: rebased on the latest 1.9.5 and committed as r385.
Original comment by dreadnaut
on 24 Mar 2013 at 4:35
Original issue reported on code.google.com by
crazy4ch...@gmail.com
on 9 Mar 2013 at 12:38