[Blacklight-development] html_head committed, but may be problems

[Jonathan Rochkind]

Hmm, I committed the html_head stuff. But it looks like there is some
weirdness, it is including all the JS files multiples times for me,
which is causing weirdness. Will have to investigate tomorrow, not sure
if this is the result of something I've done locally, or will effect
everyone, not sure why I didn't notice it in testing.

Jonathan

--
You received this message because you are subscribed to the Google Groups "Blacklight Development" group.
To post to this group, send email to blacklight-...@googlegroups.com.
To unsubscribe from this group, send email to blacklight-develo...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/blacklight-development?hl=en.

[Jonathan Rochkind]

Bah, it probably does effect everyone. Darn.

For some confusing reason involving our Engines use, the
blacklight/controllers/application.rb file is being loaded TWICE. Which
means each it's loaded, it adds it's before_filter. Which means the
before_filter gets added twice, which means it gets called twice. WHAT A
PAIN. The engines stuff is continually doing weird crap, unfortunately.
I can't wait for Rails 3.0, when hopefully this will get more reasonable.

Should I roll back the commit of the html_head thing until I figure out
a way around it? Not sure what that way is.

Jonathan

[Jonathan Rochkind]

A similar issue seems to be calling the :verify_authenticity_token
before_filter, also added in BL ApplicationController to be added THREE
times. I get that one is idempotent, so it doesn't cause a problem,
other than inefficiency. This is incredibly frustrating. It's been a
frustrating coding day for me.

[Jessie Keck]

Hi Jonathan,
This is surprising. I can't remember this happening when I had your branch checked out. Then again I don't remember it explicitly NOT happening either.

Is this only in the case of injecting css/js via the ApplicationController's before filter?

If you feel that you need to roll it back I would say go for it. However; if you believe that you will be getting cycles tomorrow to look into this and possibly fix it I think we can leave it in for the evening. It is trunk after all.

- Jessie

[Jonathan Rochkind]

I figured it out. No need to roll back I think, although we need to take
a look at what the template installer does with your
application_controller.rb.

Jessie, can you tell me if you have a LOCAL
app/controllers/application_controller.rb, and if so, does it have a
line at the top that includes/requires/loads the BL distro
application_controller.rb ins ome way?

I had at the top of my _local_ application_controller.rb, this line:

eval
File.read('vendor/plugins/blacklight/app/controllers/application_controller.rb')

I have no idea where that came from, I guess some ancient way of doing
things, which doesn't make any sense. But it's that line that caused
the BL distro application_controller to be loaded twice, causing filters
to be applied twice.

So if you have that line in your application_controller.rb, remove it!

The current template installer instead puts this at the top of your
application_controller.rb:
require
'vendor/plugins/blacklight/app/controllers/application_controller.rb'

But guess what: That causes the problem too.

If you change that to require_dependency instead, it does NOT cause the
problem. However, in my case, simply _removing_ this logic altogether
works _fine_, Engines includes the base plugin application_controller.rb
all by itself, there is no need for ANY require/require_dependency line.

I think the template needs to be changed to _either_ use
"require_dependency" there instead of "require" OR simply remove this
step altogether. I would vote for removing it altogether, since as far
as I can tell it is entirely unneccesary.

Comments, Jessie or anyone else?

Jonathan

[Jonathan Rochkind]

Further experimentation reveals that the require_dependency DOES work
better than nothing for allowing "over-rides" to work the way you think.

Any objections if I commit a fix to the template installer to make the
BL distro application_controller "require_dependency" instead of
"require" in the local application_controller.rb?

Longer term... this is all really confusing. We probably don't want to
rely on automatic Engine "mix-in" for this, but instead put the core BL
stuff in it's own module or controller class which is actually
explicitly mixed-in or inherited or what have you. Unless Rails 3.0
makes the Engine mix-in stuff less confusing.

Jonathan

[Jessie Keck]

Hey Jonathan,
The Advanced Search plugin for Blacklight only adds the require line to the app level ApplicationHelper to require the plugin's ApplicationHelper (as does Blacklight). It doesn't touch the ApplicationController.

I think the ApplicationHelper (and possibly Controller) used to use require_dependency at one point. I can't exactly remember the reasoning behind moving to require. I thought that Matt M. had done that deliberately for some reason. However; this has never worked for me w/ SearchWorks and I still use require_dependency for the ApplicationHelper and don't even have an app level ApplicationController. I get my app level methods, but they don't override the plugin's methods. Weird right?

As far as the module/controller class there is a branch that does that right now ( http://github.com/projectblacklight/blacklight/tree/app_controller_helper_modules ). It's pretty old so could probably only be useful as a proof of concept being that merging now might be a nightmare.

I'm not sure how they've improved the engines mix-ins in Rails 3.0. I guess it may be worth investigating for the longer term.

- Jessie

[Jonathan Rochkind]

[We might want to conduct further discussion on the JIRA ticket to keep
the list low traffic for people who want it such?
http://jira.projectblacklight.org/jira/browse/CODEBASE-218 ]

The Blacklight template installer definitely DOES add "require" right
now to the top of application_controller. I just tried it out from
master, it did.

In my testing, while "require" messes things up, leaving out everything
also results in somewhat confusing behavior, where additional
before_filters you add in your local application_controller end up being
applied _before_ the "inherited" before_filters, rather than after.
Which messes up some common use cases for the html_head stuff.

So my suggestion is to change the template installer to do
"require_dependency" instead of "require" as it does presently. It seems
to result in expected behavior.

Jonathan

[Jessie Keck]

I'm not saying that Blacklight doesn't touch the ApplicationController, it absolutely does. However; the Advanced Search plugin does not.

Would this also affect the ApplicationHelper?

I say we keep this on list as it affects anybody who has a Blacklight application and also in hopes of catching the eye of somebody who might have some project memory of why we went this route. ( This entire conversation has happened at one point or another in the past )

- Jessie

[Jonathan Rochkind]

Nope, my issue does not have anything to do with the ApplicationHelper
as far as I know, no change is needed there to resolve my issue.
Whether the ApplicationHelper should also be using require_dependency or
not for reasons unrelated to my issue -- I have no idea, the whole thing
is confusing.

Fine with me to keep it on list, just trying to be sensitive to those
who want less traffic on the list.