e107inc / e107

e107 Bootstrap CMS (Content Management System) v2 with PHP, MySQL, HTML5, jQuery and Twitter Bootstrap. Issue Discussion Room: https://gitter.im/e107inc/e107
https://e107.org
GNU General Public License v3.0
322 stars 214 forks source link

BOOTSTRAP constant is not defined when library bootstrap is not used with scope front #4499

Closed Jimmi08 closed 3 years ago

Jimmi08 commented 3 years ago

Bug Description

I noticed this because backcompat.css was used with the new theme.

With

    <libraries>
        <library name="bootstrap" version="4" scope="wysiwyg,admin"/>
        <library name="fontawesome" version="5" scope="wysiwyg,admin"/>
    </libraries>

BOOTSTRAP is not defined and this test fails:

if(THEME_LEGACY === true || !deftrue('BOOTSTRAP'))
{

    $e_js->otherCSS('{e_WEB_CSS}backcompat.css');
    $e_js->footerFile('{e_WEB_JS}core/backcompat.js');
}

After adding the scope "front", it was fixed.

For now, I fixed it with

    public function init()
    {
      define("BOOTSTRAP", 4);
    }

in theme class

CaMer0n commented 3 years ago

This is the expected behavior. Closing.

Jimmi08 commented 3 years ago

Oh, because of the missing front scope?

So define it in init() is correct?

Just note: Why is a theme without bootstrap considered a legacy - the name of the file is backcompat? There is THEME_LEGACY for this, BOOTSTRAP check for loading this css is not correct.
I understand you need some bootstrap classes on non-bootstrap theme too (there is a row class for example), but it should be in a separate css file and loaded if BOOTSTRAP is not defined.
But I can live with this.

Moc commented 3 years ago

If you add the scope 'front' for the bootstrap library, I'd expect that defining the BOOTSTRAP constant should not be needed anymore?

Jimmi08 commented 3 years ago

If you add the scope 'front' for the bootstrap library, I'd expect that defining the BOOTSTRAP constant should not be needed anymore?

@Moc if this question is for me, adding front scope is not an option. Bootstrap4 themes can be compiled with the bootstrap library already, mainly if you use sass. So adding scope front would mean 2x loading bootstrap library.

So the scope solution is correct. The question is how / where is the correct place to define BOOTSTRAP manually. in theme.php, in init() ?

Moc commented 3 years ago

So I guess the question then becomes what standard should we use in the scenario where bootstrap is already loaded (e.g. when using sass) to prevent double loading. Do you have an idea on this @CaMer0n?

Labelled as documentation.

CaMer0n commented 3 years ago

@Jimmi08 @Moc This is what the 'exclude' attribute is for. (see bootswatch css entries which are pre-compiled with bootstrap) :

<css file="https://maxcdn.bootstrapcdn.com/bootswatch/3.3.7/flatly/bootstrap.min.css" name="Flatly" thumbnail='images/admin_flatly.webp' scope='admin' exclude='bootstrap' />
<css file="https://maxcdn.bootstrapcdn.com/bootswatch/3.3.7/sandstone/bootstrap.min.css" name="Sandstone" thumbnail='images/admin_sandstone.webp' scope='admin' exclude='bootstrap'/>
<css file="https://maxcdn.bootstrapcdn.com/bootswatch/3.3.7/superhero/bootstrap.min.css" name="Superhero" thumbnail='images/admin_superhero.webp' scope='admin' exclude='bootstrap'/>

It prevents loading bootstrap twice, while still allowing theme.xml to tell e107 what bootstrap version is being used.

Manually defining the BOOTSTRAP constant is 100% deprecated. Just use <library>

<libraries>
        <library name="bootstrap" version="5" scope="front,wysiwyg"/>

Also, scope=admin should only ever be found in bootstrap3's theme.xml, . NO other theme should contain it, because no other theme is currently supported as an admin area theme.

Jimmi08 commented 3 years ago

@CaMer0n @Moc this is not the solution

I can't avoid loading core javascript bootstrap file this way

I want to load all bootstrap files from the local theme source, no CDNs, no core files. For example, because there is 5.0.1 version available already.

Don't tell me to use theme_library.php because it is too complicated for something like that.

There has to be a way to define constant BOOTSTRAP manually. The reason? Because you are loading template arrays according to this constant. See user settings. THEME_LEGACY should be used, not BOOTSTRAP.

if(deftrue('BOOTSTRAP'))
{
            $template = e107::getCoreTemplate('usersettings','', true, true); // always merge

 }

This is just one example, I spent almost an hour debugging what is wrong with my theme/template. So themes without bootstrap libraries should use old template variables, not arrays?

I am just trying to explain why this can't be deprecated. You need to fix way how this constant is used before...