Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use logging for apphook reload #5448

Closed
czpython opened this issue Jun 22, 2016 · 11 comments · Fixed by #6917
Closed

Use logging for apphook reload #5448

czpython opened this issue Jun 22, 2016 · 11 comments · Fixed by #6917

Comments

@czpython
Copy link
Contributor

There are some places in the apphooks reload module that use print statements instead of logging.
https://github.com/divio/django-cms/blob/develop/cms/utils/apphook_reload.py#L96
These should use logging instead.

Related to #5084

@jsma
Copy link
Contributor

jsma commented Jun 28, 2016

Was just about to open a ticket for this. I agree it should use logging as well, but I'd also like to improve what is logged so it's as useful as possible. For instance, it's very unclear why I see this kind of output or what I'm supposed to do with this information:

New revision!!!! RELOAD!
   074c568c-1ce0-49fe-afe2-1027ddcaff0b (<class 'str'>)
-> None (<class 'NoneType'>)

Or:

reverse('my_test_app_view'): Reverse for 'my_test_app_view' with arguments '()' and keyword arguments '{}' not found. 0 pattern(s) tried: []

@czpython
Copy link
Contributor Author

In the hopes someone can tackle this, here's the explanation:

When an apphook is attached to a page, if the project has the ApphookReloadMiddleware middleware installed, the cms will trigger a reload of the configured url conf module. This allows the content manager to continue to use the page with the apphook urls already applied.

Without the ApphookReloadMiddleware middleware installed, content managers would not be able to see the attached app until a sys admin restarts the wsgi process.

Example, user attaches the blog apphook to a page called blog.

If ApphookReloadMiddleware middleware is installed, content manager can start adding and editing articles, also the article detail pages would work fine like /en/blog/django-cms-part-1/

If ApphookReloadMiddleware middleware is not installed, content manager can probably add and edit articles via the admin (depending on the app) but if the content manager visits an article detail page (/en/blog/django-cms-part-1/) he'll get a 404 because the url conf module was loaded on memory when the wsgi process first started and so does not know about the article urls.

To manage this, there's a global revision and local revision records.
The global revision is stored in the database and the local one in thread.

The first message:

New revision!!!! RELOAD!
   074c568c-1ce0-49fe-afe2-1027ddcaff0b (<class 'str'>)
-> None (<class 'NoneType'>)

Tells that a new local revision is being created because a change has been detected and so url conf will be reloaded.

After the url conf module is reloaded, it's necessary to trigger a reverse() call in order to make sure django loads all included urls.

The second message:

reverse('my_test_app_view'): Reverse for 'my_test_app_view' with arguments '()' and keyword arguments '{}' not found. 0 pattern(s) tried: []

Simply informs that we've tried to reverse the my_test_app_view url and if threw an error.

@FabrizioA
Copy link

I have the same "problem"...
How can I hide this annoying message??

"""
New revision!!!! RELOAD!
8b8be4b3-1fce-469a-bb04-bca63091de22 (<type 'unicode'>)
-> None (<type 'NoneType'>)
"""

@FabrizioA
Copy link

This message is very annoying... please hide it soon!! :(

@racitup
Copy link

racitup commented Sep 28, 2016

Agreed, stuff like this should be in the debug log, info at worst
It's basically just informing the admin that it's working properly, whereas should only inform them if it's not working properly. If everything logged something on normal operation we would be in trouble!
It detracts from the django cleanliness that I have been enjoying up until now..

@stuaxo
Copy link
Contributor

stuaxo commented Aug 2, 2019

I came across this as the message is really confusing, I'm working with a codebase that has some strange corners already and the message coming from djangocms itself is the last thing I expected.

@stuaxo
Copy link
Contributor

stuaxo commented Jan 15, 2020

PR above removes the print statements.

@Aiky30
Copy link
Contributor

Aiky30 commented Dec 8, 2020

The following PR will fix this issue.

@Aiky30 Aiky30 linked a pull request Dec 8, 2020 that will close this issue
3 tasks
@stuaxo
Copy link
Contributor

stuaxo commented Dec 8, 2020

Looks like the only issue it has is a merge issue with the ChangeLog.

@yakky
Copy link
Member

yakky commented Dec 8, 2020

@stuaxo fixed, thanks for reporting

@stuaxo
Copy link
Contributor

stuaxo commented Feb 12, 2021

Thanks all.

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

Successfully merging a pull request may close this issue.

7 participants