Language Change PR-staging

Failure

Referenced as Related to: # 8055

User tests: Successful: Unsuccessful:

avatar hans2103
hans2103
20 May 2015

problem: Joomla! makes no difference in devices when caching a page.

This pull request makes an end to it and adds a prefix to the cache ID.
This pull request uses the Joomla! default browser detection implementation.

avatar hans2103 hans2103 - open - 20 May 2015
avatar mbabker
mbabker - comment - 20 May 2015

2 concerns here:

1) Method name. setDevicePrefix() implies I can set the prefix myself, in reality you're getting a prefix here.
2) To me it seems like this change really only affects sites which have a massively different mobile version compared to the desktop version. On sites using fully responsive templates and presenting the same output to all devices, there is no benefit to be gained with this change.

avatar hans2103
hans2103 - comment - 20 May 2015

Hi @mbabker
1. please feel free to suggest a better name. Perhaps another name suits better.
2. example: with this PR you have the ability to create device specific design. For instance... don't show huge background images in masthead on mobile while showing them on desktop. (The current device detection implementation of Joomla doesn't trigger tablet, bot or other devices). Even a slight difference is good enough for me to cache that different page.

avatar zero-24
zero-24 - comment - 20 May 2015
  1. please feel free to suggest a better name. Perhaps another name suits better.

maybe getDevicePrefix()?

avatar zero-24 zero-24 - change - 20 May 2015
Labels Added: PR-staging
avatar zero-24 zero-24 - change - 20 May 2015
Category Cache Libraries
avatar zero-24 zero-24 - change - 20 May 2015
Status New Pending
avatar hans2103
hans2103 - comment - 20 May 2015

Now creating an option for Global Config to enable device specific caching if Caching is activated. Will add this to this pull request.

avatar dgt41
dgt41 - comment - 20 May 2015

@hans2103 shouldn’t we first work on #6859 and then try to fix the caching? For me this looks like back and forth...

avatar hans2103
hans2103 - comment - 20 May 2015

yes and no... This pull request works with the current implementation of the mobile detect class.
When #6859 is implemented this PR can be expanded with the newly created class.

avatar Fedik
Fedik - comment - 20 May 2015

problem: Joomla! makes no difference in devices when caching a page.

to be honest I do not see the problem here, and have a doubt that Joomla realy need need it ...
any real life example?
sorry I missed your second post, but I still do not see a problem :wink:

and you sure that it will not lead to unexpected behavior mobile/non-mobile, because cache duplication?

avatar joomla-cms-bot joomla-cms-bot - change - 20 May 2015
Labels Added: Language Change
avatar hans2103
hans2103 - comment - 20 May 2015

@Fedik - just added Global Config option to enable/disable device specific caching.
Useful for website who don't differ the HTML output between mobile or non-mobile devices.

avatar Kubik-Rubik Kubik-Rubik - change - 21 May 2015
Milestone Added: Joomla! 3.5.0
avatar ggppdk
ggppdk - comment - 23 Jun 2015

I think this PR is quite useful for websites (or extensions),
that want to serve different page to mobile devices

not everything can be done in CSS

only issue that i see in this change, is that it will make cache system consume more space

avatar asika32764
asika32764 - comment - 23 Jun 2015

:+1:

avatar mbabker
mbabker - comment - 23 Jun 2015

If we're really going to pursue this (I still don't see what the benefit here is personally), I'd suggest using JApplicationWebClient ($app->client) instead of JBrowser as it is supposed to be a bit more 'intelligent' with platform detection.

avatar ggppdk
ggppdk - comment - 23 Jun 2015

about using JApplicationWebClient, the code should be (asking if i am correct)

$client = new JApplicationWebClient();
$isMobile = $client->__get('mobile');

About this topic someone can clame that is better done at the component as it should know better of what is happenning, e.g. see the following code,

  • but the website integrator can be using custom template code, or using Joomla plugins, etc, that the component does not know about, so having this inside Joomla itself, makes sense

For doing it inside the component i have an example below

function display($cachable = null, $urlparams = false)
{
    ...

    $jinput = JFactory::getApplication()->input;

    if (!empty($urlparams))
        $safeurlparams = & $urlparams;  // explicitely given
    else
    {
        $safeurlparams = array();  // Decide which URL params to add ...
        ...
    }


    // If component is serving different pages to logged users, this will avoid
    // having users seeing same page after login/logout when conservative caching is used
    if ($userid = JFactory::getUser()->get('id'))
    {
        $jinput->set('__user_id__', $userid);
        $safeurlparams['__user_id__'] = 'STRING';
    }


    // If component is serving different pages for mobile devices, this will avoid
    // having users seeing the same page regardless of being on desktop or mobile
    $client = new JApplicationWebClient();
    if ($client->__get('mobile'))
    {
        $jinput->set('__mobile__', 'YES');
        $safeurlparams['__mobile__'] = 'STRING';
    }


    // ... decide if page is cacheable, according to logic known to the component
    $cachable = ...

    parent::display($cachable, $safeurlparams);
}
avatar mbabker
mbabker - comment - 23 Jun 2015

You don't even need to instantiate it every time.

$client = JFactory::getApplication()->client;
$isMobile = $client->mobile;

Typically you won't call methods with double underscores directly (like __construct(), __get(), etc.). PHP calls those magic methods and handles using them when needed. So $client->mobile is the same as your $client->__get('mobile').

Otherwise I didn't really dig into your example, the client stuff just happened to show up immediately in the channel I'm seeing notifications from.

avatar ggppdk
ggppdk - comment - 23 Jun 2015

You don't even need to instantiate it every time.

Thanks, i wanted to avoid making detection run twice
i now see the creation of the JApplicationWebClient object inside the JApplicationWeb class

  • about __get you are again right ! thanks for the info !!

I missed this, here is some more info

For object properties that are to be very often accessed e.g. a big loop using

  • protected property plus a magic method __get(),
    will result in 5 times slower access than access using a custom "getter" function, because of the extra logic executed by PHP
    (i speak of the access, of course the property calculation will only run once)

  • thus a real getter: $client->get('mobile'), would be much faster

  • and both the above are even more slow that a public property, which of course in our case we want to avoid

of course, someone getting this and using it an big loop should store it in a variable (if he/she is aware that the property is checked and returned by a magic function)

avatar hans2103
hans2103 - comment - 23 Jun 2015

Thank you for the feedback @mbabker and @ggppdk
I've adjusted my Pull Request

avatar wojsmol
wojsmol - comment - 23 Jun 2015
avatar hans2103
hans2103 - comment - 8 Jul 2015

oh boy... I just came to a point that I need some serious help on coding.
With the last commit I notice that I repeat myself with coding.
I need some assistance with this pull request.

avatar hans2103
hans2103 - comment - 28 Jul 2015

found out that I had to set Headers vary:user-agent; to get it working.
Live implementation with Redis caching can be seen by visiting https://www.byte.nl

avatar roland-d
roland-d - comment - 1 Oct 2015

@mbabker Travis fails with an Application Instantiation Error, looking at where the test fails

$this->object->store($data, $id, $group)

I think it may have something to do with this notice:

PHP Warning: PHP Startup: Unable to load dynamic library '/home/travis/.phpenv/versions/5.6.5/lib/php/extensions/no-debug-zts-20131226/apcu.so' - /home/travis/.phpenv/versions/5.6.5/lib/php/extensions/no-debug-zts-20131226/apcu.so: cannot open shared object file: No such file or directory in Unknown on line 0
Warning: PHP Startup: Unable to load dynamic library '/home/travis/.phpenv/versions/5.6.5/lib/php/extensions/no-debug-zts-20131226/apcu.so' - /home/travis/.phpenv/versions/5.6.5/lib/php/extensions/no-debug-zts-20131226/apcu.so: cannot open shared object file: No such file or directory in Unknown on line 0

As if no caching mechanism can be loaded. Is that the case?

avatar mbabker
mbabker - comment - 1 Oct 2015

The PHP warnings have nothing to do with our Application Instantiation Error message.

avatar dgt41
dgt41 - comment - 1 Oct 2015

@roland-d can you restart travis here?

avatar brianteeman
brianteeman - comment - 1 Oct 2015

Restarted it for yoy
On 1 Oct 2015 10:59 pm, "Dimitris Grammatiko" notifications@github.com
wrote:

@roland-d https://github.com/roland-d can you restart travis here?


Reply to this email directly or view it on GitHub
#6997 (comment).

avatar hans2103
hans2103 - comment - 2 Oct 2015

next week (10 October) we have a PBF in the Netherlands. Roland and I will work on this pull request.

avatar hans2103
hans2103 - comment - 10 Oct 2015

Closing this PR in favor of #8055

avatar hans2103 hans2103 - change - 10 Oct 2015
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2015-10-10 14:10:18
Closed_By hans2103
avatar hans2103 hans2103 - close - 10 Oct 2015
avatar zero-24 zero-24 - change - 10 Oct 2015
Milestone Removed: Joomla! 3.5.0

Add a Comment

Login with GitHub to post a comment