Skip to content

New architecture ? - #16

Open
atellier2 wants to merge 12 commits into
reportico-web:masterfrom
atellier2:new_architecture
Open

New architecture ?#16
atellier2 wants to merge 12 commits into
reportico-web:masterfrom
atellier2:new_architecture

Conversation

@atellier2

Copy link
Copy Markdown
Contributor

Here is a new version where

  • PSR4 is activated
  • reportico class are moved in the src folder
  • libraries smarty, pdf, adodb are loaded by composer
  • RepporticoApp class would be used in place of global variables

@daN4cat

daN4cat commented Apr 21, 2017

Copy link
Copy Markdown
Contributor

@atellier2 this looks good, well done!

@reportico-web

Copy link
Copy Markdown
Owner

Thanks, this sounds good, i will have a play with this over the next few days to understand it.

@reportico-web

reportico-web commented Apr 23, 2017

Copy link
Copy Markdown
Owner

Hi @atellier2 , thanks for that, Ive managed to pull your branch so i can try it out. I would like to incorporate this PSR-4 mechanism. So I guess the ReporticoApp you mention needs creating since the app wont run for me without this. I cannot find a file containing ReporticoApp in the branch did you provide one?

I have a couple of questions/points ..

  1. I see you load adodb, tcpdf now with composer. My adodb, tcpdf and pchart libraries are fairly customized versions of the originals and so if we were to load the with composer, would i have to create my own git sub-module versions of these in git as say reportico/adodb, reportico/tcpdf etc? For example i think the tecniccom tcpdf has a couple of things i changed for my own purposes and i would want to keep those.

  2. Im very keen to move the global variables into App get calls... some of the global variables however line the g_menu_title are report project level and so i would plan to put those in a config file per project. However the many global config parameters found in run.php should be converted to this mechanism.

  3. I think the biggest change to users with this is the need to use composer to install, rather than unzip to a folder .. so i think i might maintain a version of the ready to go with all the vendor modules included which can be unzipped for totally standalone operation. Not sure?

@daN4cat

daN4cat commented Apr 23, 2017

Copy link
Copy Markdown
Contributor

@reportico-web

  • WRT point 1 I would say yes, you need to create your versions as those that were customised and register them to packagist.com as you did for reportico.
    Alternatively you fork the original repo for those packages, apply your changes and ask for PRs and see if they want to merge those. The advantage is that you will not need to maintain them going forward and could benefit automatically of any upgrade. But if those original repo are not maintained then it's better to have your fork and as said above create your reportico- package and blah blah.

  • WRT point 3 I saw that under vendor everything was installed so it should work if you update that dir with your customised code. What is that it's not working? Not sure I can find today the time to give a try and help...

@reportico-web

Copy link
Copy Markdown
Owner

Thanks for that .. Im clear on 1 and 3 .. i think the main issue at the moment is the missing ReporticoApp file which seems to be missing in the submitted branch but which i think @atellier2 is experimenting with for setting global config parameters

@daN4cat

daN4cat commented Apr 23, 2017

Copy link
Copy Markdown
Contributor

I can't take and run the code but based on quick code check indeed the package Reportico seems missing the ReporticoApp.

With regards to adodb, tcpdf and pchart, how old are those vs the versions currently in GitHub? It looks like there is a newer tcpdf lib repo as follow up of the original tcpdf that is dated 2016 and vs6.

@atellier2

Copy link
Copy Markdown
Contributor Author

Hello,

Sorry for the missing files. It's the first time that I commit to github ;-)

I corrected some issues on the previous comit and add support for theme. The goal is to had a better separation between code and templating. It will be a solution to introduce config, js, css files in each theme easily (and remove for example $bootstrap_styling_xxx variables from the code and keep it in the templates files.)

About your questions :

  1. Composer is a way to always have the last stable version so if you customized the libraries you have to propose your optimization or add a new reportico class which extends the library. The main question is to understand why you really need to customized theses library. As we often say : keep the standard ;-)
    Could you give me an example of customization ?

  2. You're right, my example is not relevant. I checked deeper the code and I think we should have two module. The first one which is use to manage a specific report and a second one that propose a front-end with user menus.
    With this architecture it will be more easy to integrate reportico with orther framework and applications. For example we could imagine a prestashop module with an integrated report. In this case we only need the first module. We don't care about the user menu.
    For now the session variables that you use are not clear for me ...

  3. Of course, the most simple is to include all the vendor modules directly. Composer update will only be from contributor. You have to ckeck the compatibility of reportico version with each libraries.

If it's not clear (english is not my maternal language) please update !

@reportico-web

Copy link
Copy Markdown
Owner

Hi there .. I'll pull your latest in and look in next couple of days, Ive had some personal issues to deal with so sorry to not get back to you

@daN4cat

daN4cat commented Jul 2, 2017

Copy link
Copy Markdown
Contributor

@reportico-web any news with regards to this PR?

@reportico-web

Copy link
Copy Markdown
Owner

Hi apologies, i am very busy at the moment and plan to get to this mid month. I think it requires work to handle config parameters correctly and needs the handling of the libraries. I am keen to do this ASAP

@reportico-web

reportico-web commented Jul 6, 2017

Copy link
Copy Markdown
Owner

Hi there @atellier2 , Ive loaded your latest changes in ( again apologies for not trying this sooner ) ...

I like the idea of the themes youve put in, which simplifies those awfully complicated smarty files. However, im getting not output at the moment at all when i point the browser at the reportico url. When i look in the compiled smarty files i just get a very small file with ..

<?php /* Smarty version 2.6.25-dev, created on 2017-07-06 21:46:00 compiled from bootstrap3/admin.tpl */ ?>

Any ideas?

Im debugging smary at the moment

@reportico-web

reportico-web commented Jul 6, 2017

Copy link
Copy Markdown
Owner

Hi there

I overcame my problem, issue was that the smarty library you included was a slightly older version which was not PHP 7 compatible, i changed to later version and it works.

Now I turned in run.php the reportico_ajax_mode to on to get everything run through ajax... i also moved your ASSETS folder pick up up a bit, because datatables.css wasnt being picked up.. i think this was ok.

Now I have a problem.. if i go into configure turorials or try to create project when i hit the Go button i get

Call to undefined function Reportico\get_template_path()#0 /var/www/atellier2/run.php(305): Reportico\reportico->execute() #1 {main}

Also I just commited and pushed my changes .. i didnt think they would go back into your clone, i thought i was on a fork of my own.. apologies i accidentally committed some changes under the description "updated smarty" but were nothing to do with smarty.. ill avoid that in future.

@reportico-web

Copy link
Copy Markdown
Owner

FYI Im working on a branch to remove the global variables mentioned above and configure them within the project config. I am also modifying the structure of the projects config to avoid all the constants and replace them with arrays, then i will try to merge this in with the new architecture branch .. and also try to convert the run.php to use its own config file .. i quite fancy, just like the themes to allow you to have a an array of different run config to turn on and off different options ( e.g you could have a minimal config that strips out a lot of the reportico ui.

@reportico-web

Copy link
Copy Markdown
Owner

Hi there

just to give you an update, i am in the middle of the work in this, i think ill be pushing a new branch to git in about a week. This will take the work by @atellier2 further on.... hope you can stil hold on :-)

@atellier2

Copy link
Copy Markdown
Contributor Author

I was pretty busy last months but I should will come back ;-)
Exited to test your next branch !

@daN4cat

daN4cat commented Aug 21, 2017

Copy link
Copy Markdown
Contributor

@reportico-web I can see you submitted the code to a new branch, is that fully functioning or does it require test to prove it's working fine or else?
Just trying to understand what the maturity level of that code is.

@reportico-web

Copy link
Copy Markdown
Owner

Hi there

Yes well spotted! .. that was work in progress and i just committed another lot to this new-architecture branch. I would say at the moment its close but needs a bit of work. Ive refactored the code, removed most of the globals by usings @atellier2 's ReporticoApp class. I also use this class to store the individual project config now.

So much has changed that i still need to check all the normal functionality works from creating a project to running a report. I would say at this stage that it largely works but problably will be testable in a few days.

Peter

@daN4cat

daN4cat commented Aug 22, 2017

Copy link
Copy Markdown
Contributor

Sounds good Peter and well done. Keep us posted so when something is ready for testing I can give a try and feedback if any issue. In this way we can help with the hardening.

@daN4cat

daN4cat commented Aug 27, 2017

Copy link
Copy Markdown
Contributor

I just checked the new branch, I guess having asset/ now means that all the old css/ stylesheet/ js/ javascript/ images/ are probably redundant?

@reportico-web

Copy link
Copy Markdown
Owner

Just committed some more... the project should be usabe for html reports... pdf still need a little work. To answer your question, @atellier2 has created a theme concept. So that under templates folder we have theme folders under which we collect the html template layout files, the main css for the theme. The run.php contains a setTheme call which currently sets theme to bootstrap3 ... this corresponds to templates/bootstrap3. The assets currently contains images, js files and some redundant css files, but not the css of the theme. Im still getting to grips with this and not sure howbest to sue the templates and the assets concepts. Happy for any suggestions.

On a separate note i still have to get pdf integrated properly as i think i need some customisations which are not in the tcpdf and fpd vendors own folders .. im working on tht next... hopefully wont be long for something that is final.

Peter

@reportico-web

Copy link
Copy Markdown
Owner

Hi @daN4cat ... Ive got the code to the stage where its a lot more robust. I believe you can clone the reportico-new-architecture branch, run composer update and it works as far as running tutorials and creating projects reports. It seems that the other vendor repos ( smarty, tcpdf ) seem to work without tweaks but i should really check this out.

The full composer installation can be done using

composer create-project reportico-web/reportico {FOLDER} "dev-reportico-new-architecture" --prefer-dist

I still need to resolve the assets/templates ambiguity and really give it a test, as im sure there will be some things broken.

Would appreciate any feedback if you get a chance to test it and Id also really appreciate @atellier2 's comments on this integration as im using this opportunity to teach myself composer and PSR!

@daN4cat

daN4cat commented Sep 3, 2017

Copy link
Copy Markdown
Contributor

@reportico-web hope to find some time in next days to test a bit. Currently I'm busy with another feature for the OSPOS application I'm contributing to and I want to integrate Reportico to.

Many thanks for the work done, looks promising. Keep it up!

@atellier2

Copy link
Copy Markdown
Contributor Author

I do some tests and it's a very good job !!! What a big step done !
I have some very little comment or little modification. I will send the patch in a few time.

@reportico-web

Copy link
Copy Markdown
Owner

Thanks Glad you like it..will happily merge your new code .. ... I've got some questions on where to go next in terms of re architecture in order to make this the next official release . .. I'm on holiday until next week but I'd like to get ideas on things .. should the report output template be more template driven where it's easy to change report output format in templaterms. ..should twig be used as template library so it's similareally to laravel.. I'd like to produce a configuration mechanism for setting up a series of default configurations for different scenarios like a run menu configuration or a run single report configuration or run grids configuration. . User could set up different scenarios ...also plan to generate a project drop down menu or static menu editor ...etc appreciate any commentsuch and advice

@atellier2

Copy link
Copy Markdown
Contributor Author

Just some misc. patch to improve code quality ;-) and style !

Misc Patchs.zip

@atellier2 atellier2 closed this Sep 6, 2017
@atellier2 atellier2 reopened this Sep 6, 2017
@reportico-web

Copy link
Copy Markdown
Owner

Hi @atellier2, @daN4cat ive been really busy and have been dying to get back to this as i want to this new composer architecture setup on the master branch .. @atellier2 ive patched in your changes and comitted and pushed ..

I notice the monolog addition and the use of Reportico\ReporticoLog::activeDebugMode(); .. can you tell me how this works as it sounds useful?

@reportico-web

Copy link
Copy Markdown
Owner

Hi @daN4cat,@atellier2, very pleased you pushed reportico down the composer route...I just used it to require composer project phantomJS so I can generate PDF reports straight from HTML.. still in progress but you can try yourselves from new branch new-architecture-phantom using

composer create-project reportico-web/reportico <FOLDER> "new-architecture-phantom" --prefer-dist
note in run.php file use of

$q->pdf_engine = "phantomjs";

Ok next plans im planning to convert from smarty to twig engine, allow templates to control the complete layout for report output

@daN4cat

daN4cat commented Nov 25, 2017

Copy link
Copy Markdown
Contributor

Hi there,

I'm sorry if I didn't come back to this in recent months but I've been quite busy and the small time I have a try to push forward the other open source project I'm involved.

When I find the time I want to come back to this and test the new arch & composer stuff. Not sure it's completed or there is more to do?

@reportico-web

Copy link
Copy Markdown
Owner

Hi dan4cat

No worries, ive added the improved PDF generation and im currently restructuring the HTML report output to be generated through a twig template so you can in effect modify the layout of the html easilty. Then im going to announce it as a beta release for people to try. Its been such a change that im taking time before announcing anything.

Please let me know if you need any help testing anything or trying this out

Peter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants