Created attachment 164381 [details] [review]
Bug 35716: PoC

**K-T-D TEST ONLY**
At /kohadevbox/koha, run the following:
- sudo cpan File::Copy::Recursive
- sudo rm -rf /usr/share/koha
- sudo perl Makefile.PL # press Enter on all prompts
- sudo make install
- find /usr/share/koha -name hello.js
Notice the output:
/usr/share/koha/opac/htdocs/opac-tmpl/js/hello.js
/usr/share/koha/intranet/htdocs/intranet-tmpl/js/hello.js

My first attemp was using the file_map that is used to PM
but I failed. Even changing the code to allow for multiple targets
in the file_map, the WriteMakefile will always only map 1 to 1
from the PM hashref. But PL_FILES allows for an array of destinations
given one source.

This patch needs more work as a SHARED_TMPL_DIR env variable
would be ideal, amongst other improvements, but this is just a proof of
concept to have others look at and see if they agree with this approach.

Changing to NSO to get more eyes on it.

I love the idea here - it's frustrating that we have to maintain two sets of files for the same purposes - this could go along way towards standardizing more of what we do

I love the direction here though I wonder what you mean by a SHARED_TMPL_DIR environment variable?

I wonder if we could perhaps use some symlinking for development so one doesn't have to run make to test changes to these files?.. not sure if there would be a performance implications in live running using symlinks Vs hard files either on that front.

Thanks for raising this again Pedro.. sure we must be able to resolve this age old problem by now

https://gitlab.com/koha-community/Koha/-/blob/main/C4/Templates.pm?ref_type=heads#L52

I feel like we could do something here perhaps.. so instead of make copying to two places, we have a shared directory at the top level where we look for included including translated ones after we've looked in the more localised for themes version of the same files?

I think this is a great idea! It makes no sense to me to have duplicates, and then need to remember to update in two places when changes are made.

If they only relate to the staff and OPAC templates, maybe just having a shared folder under koha-tmpl would be sufficient (as proposed in comment #1?

Bearing in mind I don't understand all the nuances (such as for translation, or custom templates) , maybe that is too simplistic...

Great idea.

I suggest we use the name "common" in stead of share that can mislead to file sharing over network.
A bit like packages name like "koha-common", "mysql-common".

Koha code organisation (or lack thereof) is one of my pet peeves/favourite topics, so keen to put in my 2 cents! (I'm sorry in advance haha.)

--

If we have a /kohadevbox/koha/koha-tmpl/shared-tmpl, then it will just work for dev/git deployments, because DocumentRoot for both OPAC and Staff Interface is /kohadevbox/koha/koha-tmpl

For other installations like package installs, the DocumentRoot is different:
DocumentRoot /usr/share/koha/opac/htdocs
DocumentRoot /usr/share/koha/intranet/htdocs

But there are options. One of them is a Make approach (coded using Makefile.PL). Another option is symlinks. 

Another option is Apache configuration. We could modify the following files:
/etc/koha/apache-shared-opac.conf
/etc/koha/apache-shared-intranet.conf

We'd just need to add the following:

Alias /shared-tmpl /usr/share/koha/<wherever we want to install shared-tmpl>

Consider this URL path: /intranet-tmpl/lib/jquery/jquery-ui-1.13.2.min_23.1200019.css

That could easily be something like /shared-tmpl/lib/jquery/jquery-ui-1.13.2.min_23.1200019.css

Of course, I think this actually represents an opportunity to take it further!

"tmpl" stands for templates, and templates are processed server-side. They shouldn't be web-facing! So let's think of a new structure!

E.g.
Git:
"/kohadevbox/koha/public/shared/static"

Package install:
/usr/share/koha/public/shared/static

Apache:
Alias /shared /usr/share/koha/public/shared

--

Eventually, we'd have "/public/opac" and "/public/admin" as well. (One of the advantages of using the "public" directory name is that it reminds you that whatever is located there will be served publicly.)

These would contain "static" and "cgi-bin" directories for our static assets and CGI scripts. 

And the "templates" would move to /usr/share/koha/templates (or /tt or /tmpl or whatever), so they're not web/internet-facing. They're just available server-side as they should be.

One day, "C4" and "Koha" will move into the "lib" folder in the git source tree as well. 

This is a code organisation which is super common elsewhere and I use it on lots of other apps. 

In the long-run, it would make it much easier to develop/maintain and securely run Koha. 

Eventually, the contents of /kohadevbox/koha and /usr/share/koha will be more closely aligned. (Obviously, only the /kohadevbox/koha will have things like "t", "xt", etc.)

In summary, I think the "make" based approach would work, but I'd like to be more adventurous. 

We often let inertia carry us forward on a particular path, but I think this would be a really good opportunity to try something new. 

Anyway, just my 2 cents.

(In reply to David Cook from comment #8)
> Another option is Apache configuration. We could modify the following files:
> /etc/koha/apache-shared-opac.conf
> /etc/koha/apache-shared-intranet.conf

Alternatively, we could use /etc/koha/apache-shared.conf, but I was thinking about how koha-gitify already touches these files, so it would be easier to work with them for now.

I love your comments David, I think it's a brilliant idea. Would you be keen in submitting a patch? Maybe we start with the shared-tmpl and then move to the /public approach? I'd be onboard with whatever approach, as long as it stops us having to duplicate the same code that's used by staff and opac.

I'm coming back to this bug because I could really make use of this for bug 38340, as there's quite a few shared .inc files and one .js file there that will have to make their way to intranet-tmpl and opac-tmpl as duplicated assets, but if we had this that'd no longer be the case.

(In reply to Pedro Amorim from comment #12)
> I love your comments David, I think it's a brilliant idea. Would you be keen
> in submitting a patch? Maybe we start with the shared-tmpl and then move to
> the /public approach? I'd be onboard with whatever approach, as long as it
> stops us having to duplicate the same code that's used by staff and opac.

I'd love to submit a patch, but... it's fairly low on my priorities at the moment 😬

I'd say late December would be the earliest I could probably look at this

Looking at this again briefly as I find it interesting...

Apache config approach:
- This would be easy to implement using the Debian packages and it would "just work"
- However, any other Koha installation would require manual work to upgrade their Apache configuration, and it wouldn't be obvious what needs to be done (but then that's going to always be a problem with any non-package upgrade in terms of the Apache...)
- Automagically will work with ktd if we implement via these files: 
/etc/koha/apache-shared-opac.conf
/etc/koha/apache-shared-intranet.conf

Symlinks:
- Similarly easy to do with Debian packages
- Similarly won't work for other install types. Would require manual upgrade work
- Won't work in ktd without extra effort

Make:
- This would probably involve being a bit hacky and duplicating the code into opac and intranet targets...
- Won't work in ktd because we're pre-make at this stage

--

Overall, I think that really narrows it down to the Apache config only stragegy

Actually other things to consider...

We'd potentially need a "sharedhtdocs" key in koha-conf.xml, and koha-conf.xml doesn't get auto upgraded at this point. (That said, it's a bit silly that we have opachtdocs I reckon. Surely we should just have an htdocs base/root dir config...)

koha-translate and translation tools would need to take into account this new SHARED_TMPL directory. 

There's probably other stuff I'm forgetting about.

Sorry, Pedro, but given the amount of work this is going to take... I don't think that I'll be working on it at all. 

There's too many other things with higher impact for me to work on, unfortunately. 

I think the update to koha-conf.xml is going to be the biggest blocker...

--

If we could work out the koha-conf.xml problem, then I would say "./koha-tmpl/shared/vendor/" would be a good start, because then we could pop in all kinds of language-independent third-party/vendor libraries without worrying about translations at all...

How good would it be if we had something like Catalyst's $c->uri_for()? 

Then we wouldn't need to worry about the koha-conf.xml. 

Anyway time for an extremely late lunch...

(In reply to David Cook from comment #17)
> How good would it be if we had something like Catalyst's $c->uri_for()? 

Then again... we don't really have an application root when we're using Plack... 

- Perl is proxied from Apache "/cgi-bin/koha" to Starman "/opac" or "/intranet"
- Static assets are served by Apache from "/"

If we were to use a proper version of $c->uri_for(), we'd need to serve both from the same root. (I do this with other apps that use Catalyst. I serve /app_root/static from Apache and /app_root from Catalyst, and then I can use $c->uri_for() to build all my self-referential URLs, and easily mount the app at whatever path I want.)

--

But I guess for a proof-of-concept here we could get away with not having anything in koha-conf.xml

We could just add something into Koha/Template/Plugin/Asset.pm. Just need to refactor the "url" function there. We could even refactor it to be automagical if we really wanted to...

(In reply to David Cook from comment #16)
> Sorry, Pedro, but given the amount of work this is going to take... I don't
> think that I'll be working on it at all. 

Well, clearly I didn't stick to that. 

But even refactoring Koha/Template/Plugin/Asset.pm... we need to know the application file system route if we're going to find the file as part of the algorithm used there...

Which I guess means maybe adding another parameter to "Asset.url()" which takes a value like 'shared', which doesn't do a filesystem lookup against the application file root and instead just serves it from the web root.

(In reply to David Cook from comment #19)
> (In reply to David Cook from comment #16)
> > Sorry, Pedro, but given the amount of work this is going to take... I don't
> > think that I'll be working on it at all. 
> 
> Well, clearly I didn't stick to that. 
> 
> But even refactoring Koha/Template/Plugin/Asset.pm... we need to know the
> application file system route if we're going to find the file as part of the
> algorithm used there...
> 
> Which I guess means maybe adding another parameter to "Asset.url()" which
> takes a value like 'shared', which doesn't do a filesystem lookup against
> the application file root and instead just serves it from the web root.

Catalyst looks like it's a bit sneaky with how it finds its "home" and "root" (for templates). 

I don't want to get autobanned by Bugzilla so I won't paste links...

Catalyst.pm#L3584 and Catalyst/Utils.pm#L204

We could use __FILE__ to get /kohadevbox/koha/Koha/Template/Plugin/Asset.pm or /usr/share/koha/lib/Koha/Template/Plugin/Asset.pm

Using their algorithm we could then pop off Koha/Template/Plugin/Asset.pm and "lib" if it's present which gives us "/usr/share/koha" or "/kohadevbox/koha" which... doesn't help us.

I reiterate about how Koha's file structure is one of my pet peeves...

(In reply to David Cook from comment #16)
> Sorry, Pedro, but given the amount of work this is going to take... I don't
> think that I'll be working on it at all. 

After all that... I'm going back to this haha. 

This was a huge waste of my time today 🤣. Maybe not a waste of time. I suppose it shows that there's a lot to consider with this one especially with our different file layouts/deployments...

(In reply to David Cook from comment #21)
> This was a huge waste of my time today 🤣. 

I disagree haha
This is clearly a complex issue with numerous potential implications. I lack the required expertise here to develop an effective solution quickly so your input and thoughts are very much welcome.
I still very much believe this should be a high priority thing to fix and the long term gains are many.

(In reply to Pedro Amorim from comment #22)
> (In reply to David Cook from comment #21)
> > This was a huge waste of my time today 🤣. 
> 
> I disagree haha

Haha thanks, Pedro. Means a lot to me :).

> This is clearly a complex issue with numerous potential implications. I lack
> the required expertise here to develop an effective solution quickly so your
> input and thoughts are very much welcome.
> I still very much believe this should be a high priority thing to fix and
> the long term gains are many.

Since it's fresh in my mind now, I want to keep working on this, but I do have some other things I need to take care of. Let's see how fast I can bang something together...

Of course, the further down the rabbithole we go, the more issues we find...

Initially, I'm able to get a shared directory working fine in koha-testing-docker, but then we start looking to builds/deployments and the future...

--

/etc/koha/apache-shared-intranet-git.conf is created from /etc/koha/apache-shared-intranet.conf, so if we have 'Alias "/public" "/usr/share/koha/public"' in that file, it will follow through into koha-testing-docker. 

Which means that once koha-testing-docker is using a newer /etc/koha/apache-shared-intranet-git.conf file, Apache will start serving the installed assets rather than the git assets. Boooo.

Of course, it wouldn't be the first time I've needed to update koha-gitify, so that's not necessarily a drama...

Since we'll need to update it, it just makes me think about optimal configurations...

Created attachment 174481 [details] [review]
Bug 35716: (alternate) Add a "public" directory to Koha and share its "shared" subdir

This change adds a "public" directory to Koha. Using Apache configuration,
we share "public/shared" as "/shared", which allows us to serve the same
static content for both the OPAC and the staff interface.

At this time, this must be non-translated static assets, such as CSS, vendor
Javascript libraries, images, or other code which does not deal with
translatable strings.

Test plan:
1. Apply the patch and koha-plack --restart kohadev

2. Make sure you're using a correct koha-gitify
NOTE: See https://gitlab.com/koha-community/koha-gitify/-/merge_requests/3
NOTE: Need to modify koha-gitify for rewriting apache-shared-*.conf files
NOTE: 'Alias /shared "/usr/share/koha/public/shared"' => qq#Alias /shared "$gitcheckout/public/shared"#,

3. Setup Apache configuration
sudo cp debian/templates/apache-shared-opac.conf /etc/koha/apache-shared-opac.conf
sudo cp debian/templates/apache-shared-intranet.conf /etc/koha/apache-shared-intranet.conf
sudo rm /etc/koha/apache*git.conf
sudo /kohadevbox/gitify/koha-gitify kohadev /kohadevbox/koha
sudo service apache2 restart

4. Go to http://localhost:8081/cgi-bin/koha/mainpage.pl
5. Open the console, and run the following:
debug_shared_js();
6. Note that the console log shows "You successfully ran this function!"

7. Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
8. Open the console, and run the following:
debug_shared_js();
9. Note that the console log shows "You successfully ran this function!"

10. Check Koha installations
--
1. Check a "single" build
NOTE: For simplicity, run the following as the root user:
make clean
rm -rf /opt/kohatest/
INSTALL_MODE=single INSTALL_BASE=/opt/kohatest PERL_MM_USE_DEFAULT=1 perl Makefile.PL
make
make install

vi /opt/kohatest/etc/koha-httpd.conf
Note that the 'shared' alias is there:
Alias /shared "/opt/kohatest/public/shared"

2. Check a "dev" build
NOTE: For simplicity, run the following as the root user:
make clean
rm -rf /opt/kohatest/
INSTALL_MODE=dev INSTALL_BASE=/opt/kohatest PERL_MM_USE_DEFAULT=1 perl Makefile.PL
make
make install

vi /opt/kohatest/etc/koha-httpd.conf
Note that the 'shared' alias is there:
Alias /shared "/kohadevbox/koha/public/shared"

3. Check a "standard" build
NOTE: For simplicity, run the following as the root user:
make clean
rm -rf /opt/kohatest/
INSTALL_MODE=standard INSTALL_BASE=/opt/kohatest KOHA_USER=root PERL_MM_USE_DEFAULT=1 perl Makefile.PL
make
make install

vi /etc/kohatest/koha-httpd.conf
Note that the 'shared' alias is there:
Alias /shared "/opt/kohatest/public/shared"

Created attachment 174482 [details] [review]
Bug 35716: (alternate) Tidy

QA tools aren't happy, but fixing the tidiness on Makefile.PL and rewrite-config.PL would require a HUGE tidying effort. I don't think it's worth it. 

QA tools also aren't happy with what I've done for the intranet-bottom.inc, opac-bottom.inc, and public/shared/js/debug.js but that's fine for now. Those are really just for a proof-of-concept at this point. We could easily leave off those files and Koha/Template/Plugin/Asset.pm and just have this first change be a structural one. In fact, that's probably preferable anyway...

Created attachment 174483 [details] [review]
Bug 35716: (alternate) Add a "public" directory to Koha and share its "shared" subdir

This change adds a "public" directory to Koha. Using Apache configuration,
we share "public/shared" as "/shared", which allows us to serve the same
static content for both the OPAC and the staff interface.

At this time, this must be non-translated static assets, such as CSS, vendor
Javascript libraries, images, or other code which does not deal with
translatable strings.

Test plan:
1. Apply the patch and koha-plack --restart kohadev

2. Make sure you're using a correct koha-gitify
NOTE: See https://gitlab.com/koha-community/koha-gitify/-/merge_requests/3
NOTE: Need to modify koha-gitify for rewriting apache-shared-*.conf files
NOTE: 'Alias /shared "/usr/share/koha/public/shared"' => qq#Alias /shared "$gitcheckout/public/shared"#,

3. Setup Apache configuration
sudo cp debian/templates/apache-shared-opac.conf /etc/koha/apache-shared-opac.conf
sudo cp debian/templates/apache-shared-intranet.conf /etc/koha/apache-shared-intranet.conf
sudo rm /etc/koha/apache*git.conf
sudo /kohadevbox/gitify/koha-gitify kohadev /kohadevbox/koha
sudo service apache2 restart

4. Go to http://localhost:8081/shared/.placeholder
Note that there's a file there

5. http://localhost:8080/shared/.placeholder
Note that it's the same file as the staff interface

6. Check Koha installations
--
1. Check a "single" build
NOTE: For simplicity, run the following as the root user:
make clean
rm -rf /opt/kohatest/
INSTALL_MODE=single INSTALL_BASE=/opt/kohatest PERL_MM_USE_DEFAULT=1 perl Makefile.PL
make
make install

vi /opt/kohatest/etc/koha-httpd.conf
Note that the 'shared' alias is there:
Alias /shared "/opt/kohatest/public/shared"

2. Check a "dev" build
NOTE: For simplicity, run the following as the root user:
make clean
rm -rf /opt/kohatest/
INSTALL_MODE=dev INSTALL_BASE=/opt/kohatest PERL_MM_USE_DEFAULT=1 perl Makefile.PL
make
make install

vi /opt/kohatest/etc/koha-httpd.conf
Note that the 'shared' alias is there:
Alias /shared "/kohadevbox/koha/public/shared"

3. Check a "standard" build
NOTE: For simplicity, run the following as the root user:
make clean
rm -rf /opt/kohatest/
INSTALL_MODE=standard INSTALL_BASE=/opt/kohatest KOHA_USER=root PERL_MM_USE_DEFAULT=1 perl Makefile.PL
make
make install

vi /etc/kohatest/koha-httpd.conf
Note that the 'shared' alias is there:
Alias /shared "/opt/kohatest/public/shared"

Created attachment 174484 [details] [review]
Bug 35716: (alternate) Javascript proof of concept

There we go. Put that together with my pull request to the community's koha-gitify, and we've got a fully functioning shared path between the OPAC and the staff interface.

Koha/Template/Plugin/Asset.pm would only need a little bit more refactoring to make the QA tools happy. 

The core architectural change is the thing I'm most pleased with.

Hope you enjoy it, Pedro ;)

(One day, I hope we'd be able to move DocumentRoot over to /usr/share/koha/public and just serve all static assets from there, and koha-tmpl would just be for template files.)

Hi David, for what it's worth, I'm blown away by this work. Thank you so much for following up. I won't pretend to understand everything that's going on here, but this sure looks promising and far superior to my initial PoC.

I'm able to follow the test plan successfully up to step 5, but for step 6 onwards, the 'make install' command is failing for me with the error:
chown: invalid user: ‘koha:koha’
make: *** [Makefile:12431: set_koha_ownership] Error 1

I believe this is because the Makefile tries to chown to koha:koha but ktd's user:group is kohadev-koha:kohadev-koha. Please let me know what I'm missing here if you can. The error I'm encountering is possibly not even related to this work (?).

I'm not as familiarized with all the different Koha builds options as you are. What should be our next steps here to ensure a seamless upgrade path for existing installations?

(In reply to Pedro Amorim from comment #32)
> Hi David, for what it's worth, I'm blown away by this work. Thank you so
> much for following up. I won't pretend to understand everything that's going
> on here, but this sure looks promising and far superior to my initial PoC.

Thanks, Pedro. That means a lot to me, and I'm happy to explain anything you'd like.

> I'm able to follow the test plan successfully up to step 5, but for step 6
> onwards, the 'make install' command is failing for me with the error:
> chown: invalid user: ‘koha:koha’
> make: *** [Makefile:12431: set_koha_ownership] Error 1
> 
> I believe this is because the Makefile tries to chown to koha:koha but ktd's
> user:group is kohadev-koha:kohadev-koha. Please let me know what I'm missing
> here if you can. The error I'm encountering is possibly not even related to
> this work (?).

That error should only appear for the "standard" build. Are you saying you have it for all the builds. 

In any case, you should be able to get around it by setting the KOHA_USER and KOHA_GROUP environmental variables to "kohadev-koha" and "kohadev-koha".

I just ran as root to be lazy. 

> I'm not as familiarized with all the different Koha builds options as you
> are. What should be our next steps here to ensure a seamless upgrade path
> for existing installations?

Package installations will just automagically work.

Tarball installations will require manual intervention to the Apache config, but it's the same thing needed when ERM was introduced. 

Git installs will just need to make sure they have the correct version of koha-gitify, and will need to re-generate some files (as I mention in the test plan).

I think most installations are package installations, so I think overall it should go smoothly.

(In reply to David Cook from comment #33)
> > I'm able to follow the test plan successfully up to step 5, but for step 6
> > onwards, the 'make install' command is failing for me with the error:
> > chown: invalid user: ‘koha:koha’
> > make: *** [Makefile:12431: set_koha_ownership] Error 1
> > 
> > I believe this is because the Makefile tries to chown to koha:koha but ktd's
> > user:group is kohadev-koha:kohadev-koha. Please let me know what I'm missing
> > here if you can. The error I'm encountering is possibly not even related to
> > this work (?).
> 
> That error should only appear for the "standard" build. Are you saying you
> have it for all the builds. 
> 
> In any case, you should be able to get around it by setting the KOHA_USER
> and KOHA_GROUP environmental variables to "kohadev-koha" and "kohadev-koha".
> 
> I just ran as root to be lazy. 

Actually because of the chown I guess you'd have to be running as root/with sudo anyway. 

But yeah looking at https://github.com/Koha-Community/Koha/blob/main/Makefile.PL#L1706 it does look like set_koha_ownership is a noop except for the "standard" install mode. 

And then only if you're not using KOHA_USER=root, which I did put in the test plan, so you should be fine?