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

Added assets versioning #12591

Closed
wants to merge 1 commit into from

Conversation

@Jurigag
Copy link
Member

commented Feb 1, 2017

Hello!

  • Type: new feature
  • Link to issue: #639

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the Contributing Guidelines?
  • I have checked that another pull request for this purpose does not exist.
  • I wrote some tests for this PR.

Small description of change: This change is adding assets versioning setting it either in resource or collection, resource version overrides collection version with option for automatic assets versioning using filemtime.

Thanks

@Jurigag Jurigag force-pushed the Jurigag:3.1.x-assets-versioning branch 3 times, most recently from 356cfc7 to 8bf8fb3 Feb 1, 2017

@Jurigag

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2017

Also i think Manager::output needs in future some recator cuz it looks like it have duplicated code.

@Jurigag Jurigag force-pushed the Jurigag:3.1.x-assets-versioning branch from 8bf8fb3 to 357d09e Feb 1, 2017

}
let prefixedPath = prefixedPath . "?ver=" . version;
}
}

This comment has been minimized.

Copy link
@sergeyklay

sergeyklay Feb 1, 2017

Member

I dislike such pasta code

This comment has been minimized.

Copy link
@Jurigag

Jurigag Feb 1, 2017

Author Member

Oh so let prefixedPath .= "?ver=" . version ?

This comment has been minimized.

Copy link
@sergeyklay

sergeyklay Feb 1, 2017

Member

Ideologically the version must be a string or null. Not boolean or yet another type.

This comment has been minimized.

Copy link
@Jurigag

Jurigag Feb 1, 2017

Author Member

I mean i wanted true/false to make it automatic versioning - so version is file modification time as you can see in this code. Don't really see a point in adding seperated method for version from filemtime or something like this.

This comment has been minimized.

Copy link
@sergeyklay

sergeyklay Feb 1, 2017

Member

So do you prefer to introduce an entity that means anything instead of 2 specialized methods

This comment has been minimized.

Copy link
@Jurigag

Jurigag Feb 1, 2017

Author Member

Well yea, cuz this will require to https://github.com/phalcon/cphalcon/pull/12591/files#diff-f14ebcff6721de771f55f77e6f962321R96 have another argument too. I mean maybe your solution is better, i can change it, just felt it will be better to have just one method/argument added which can be used for both stuff.

This comment has been minimized.

Copy link
@Jurigag

Jurigag Feb 2, 2017

Author Member

@Green-Cat @SidRoberts @Izopi4a @wolftankk @akatasonov what you think above another method?

This comment has been minimized.

Copy link
@Izopi4a

Izopi4a Feb 2, 2017

Contributor

i find this PR very useful. maybe if this pasta is ugly, we can like in volt services..


$di->set('assets', function(){
  $cls = new Assets(); 
  $cls->setAppendString("?ver=".$config->assetsVersion);
  
  return $cls;
}

but as i said, versioning to assets its very important for me.

both cases, i dont think it should matter that much, its nothing un debuggable, its not super hard to understand and we can always change it if some person explain it sux

This comment has been minimized.

Copy link
@Jurigag

Jurigag Feb 2, 2017

Author Member

I don't like append string. What with auto versioning? What with other version for certain resource etc? I can add other argument and method but it will mean that there will be additional argument - and as you can see in current code it's not necessary really. Shouldn't we keep things simple, stupid instead of adding something which is not needed?

}
let prefixedPath = prefixedPath . "?ver=" . version;
}
}

This comment has been minimized.

Copy link
@sergeyklay

sergeyklay Feb 1, 2017

Member

The same here

@david-duncan

This comment has been minimized.

Copy link

commented Feb 1, 2017

I try to refrain from commenting if it's not constructive but this really doesn't belong in the framework.

@Jurigag

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2017

Why it doesn't belong in the framework? There is assets manager in framework so why not add versioning to it which is pretty need feature i think(like we wan't people to refresh their js/css)?

@sergeyklay

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

I prefer a more flexible approach. For example Version Strategy (incremental, direct, etc). A separated entity with delegated responsibility.

@Jurigag

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2017

How this is not flexible? For resource versioning it depends hardly on Resource itself, cuz in resource is uri generated for certain resources, as well uri for joined resources is generated in collection.

@Jurigag Jurigag force-pushed the Jurigag:3.1.x-assets-versioning branch 2 times, most recently from b3576ee to 5428fdb Feb 2, 2017

@sergeyklay

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

What we can have:

  • Incremental strategy
  • Explicit strategy
  • md5/crc32 (from content)
  • Timestamp strategy

Do you really want to incapsulate all this in one optional parameter and introduce a pasta factory for it?

@Jurigag

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2017

md5/crc32 from content is really bad way for performance. Some example for incremental/explict strategy, what you mean by them? It can't obviously increment itself.... You can either provide version by hand like 1.0.0 setting version or to tell framework to generate it from timestamp. How it supposed to increment version?

Right now incremental, explict and timestamp is implemented with this code so i don't see a problem.

@sergeyklay

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

The problem is not that something isn't done. I mean design and single responsibility.

@sergeyklay

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

Do you really like such design?

if version != null {
    if version {
        if version === true {
            // ...
        }
    }
}

@Jurigag Jurigag force-pushed the Jurigag:3.1.x-assets-versioning branch from 5428fdb to 57d502e Feb 2, 2017

@Jurigag

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2017

So you like more something like:

$assets->addJs(PATH_DATA.'assets/script1.js', true, false, null, '1.0.0'); // 1.0.0 version
$assets->addJs(PATH_DATA.'assets/script1.js', true, false, null, null, true); // version from timestamp
$assets->addJs(PATH_DATA.'assets/script1.js', true, false, null, '1.0.0', true); // and here we should have what and why?

?

@sergeyklay

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

I will keep this PR open for a some time to allow other contributors a chance to speak up, and to take a fresh look at it later.

@Jurigag Jurigag force-pushed the Jurigag:3.1.x-assets-versioning branch 2 times, most recently from 70647a0 to d2b7d2b Feb 2, 2017

@Jurigag

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2017

Failing tests seems unrelated.

@Jurigag Jurigag closed this Feb 2, 2017

@Jurigag Jurigag reopened this Feb 2, 2017

@sergeyklay sergeyklay added this to the 3.2.0 milestone Mar 8, 2017

@sergeyklay sergeyklay force-pushed the phalcon:3.1.x branch from 928502b to b0a7493 Mar 9, 2017

@sergeyklay sergeyklay force-pushed the phalcon:3.1.x branch from f115731 to c7de98d Mar 21, 2017

@Jurigag Jurigag changed the base branch from 3.1.x to 3.2.x Mar 23, 2017

@Jurigag Jurigag force-pushed the Jurigag:3.1.x-assets-versioning branch from d2b7d2b to deb303e Mar 24, 2017

@Jurigag Jurigag changed the base branch from 3.2.x to 3.4.x May 28, 2018

@Jurigag Jurigag force-pushed the Jurigag:3.1.x-assets-versioning branch from ba01336 to 3364d85 May 28, 2018

@Jurigag Jurigag changed the base branch from 3.4.x to 4.0.x May 28, 2018

@Jurigag

This comment has been minimized.

Copy link
Member Author

commented May 28, 2018

Rebased onto 4.0.x, let me see if tests pass because i don't remember.

@Jurigag Jurigag closed this May 28, 2018

@Jurigag Jurigag reopened this May 28, 2018

@sergeyklay

This comment has been minimized.

Copy link
Member

commented May 28, 2018

@Jurigag There is few issues with 4.0.x branch. Let me fix them and update the branch

@sergeyklay sergeyklay closed this May 30, 2018

@sergeyklay sergeyklay reopened this May 30, 2018

@sergeyklay sergeyklay modified the milestones: 3.4.x, 4.0.0 May 30, 2018

@Jurigag

This comment has been minimized.

Copy link
Member Author

commented May 30, 2018

I guess i need to update tests. I will do it in a few days.

@sergeyklay

This comment has been minimized.

Copy link
Member

commented May 30, 2018

@Jurigag Assets auto versioning with collection doesn't work correctly. Take a look at
Phalcon\Test\Unit\Assets\ManagerTest::testAssetsAutoVersioningCollection:

- Expected
+ Actual
@@ @@
'<script type="text/javascript" src="/home/travis/build/phalcon/cphalcon/tests/_data/assets/script1.js?ver=1.0.0"></script>\n
-<script type="text/javascript" src="/home/travis/build/phalcon/cphalcon/tests/_data/assets/script2.js?ver=1527660806"></script>\n
+<script type="text/javascript" src="/home/travis/build/phalcon/cphalcon/tests/_data/assets/script2.js"></script>\n
#1  /home/travis/build/phalcon/cphalcon/tests/unit/Assets/ManagerTest.php:714
#2  Phalcon\Test\Unit\Assets\ManagerTest->Phalcon\Test\Unit\Assets\{closure}
#3  /home/travis/build/phalcon/cphalcon/tests/unit/Assets/ManagerTest.php:715
#4  Phalcon\Test\Unit\Assets\ManagerTest->testAssetsAutoVersioningCollection
@sergeyklay

This comment has been minimized.

Copy link
Member

commented May 30, 2018

Also please use CHANGELOG-4.x and do not change CHANGELOG.md

* Should version be determined from file modification time
* @var bool
*/
protected _autoVersion = false { set };

This comment has been minimized.

Copy link
@sergeyklay

sergeyklay May 30, 2018

Member

I propose to not introduce anymore _prefixed variables. Lets use autoVersion instead of _autoVersion.

/**
* Phalcon\Assets\Resource constructor
*/
public function __construct(string type, string path, boolean local = true, boolean filter = true, array attributes = [])
public function __construct(string type, string path, boolean local = true, boolean filter = true, attributes = null, var version = null, var autoVersion = null)

This comment has been minimized.

Copy link
@sergeyklay

sergeyklay May 30, 2018

Member

@Jurigag Do we really need to change attributes type? The Phalcon v4 will have a lot of BC incompatible changes. Could you please try to sort out how to avoid yet another

let version = this->_version;

if this->_autoVersion && this->_local {
let modificationTime = filemtime(this->getRealSourcePath());

This comment has been minimized.

Copy link
@sergeyklay

sergeyklay May 30, 2018

Member

the filemtime is I/O function, which will be called for each request. right?

This comment has been minimized.

Copy link
@StudioMaX

StudioMaX May 30, 2018

Note: The results of this function are cached. See clearstatcache() for more details.
https://secure.php.net/filemtime#refsect1-function.filemtime-notes

This comment has been minimized.

Copy link
@sergeyklay

sergeyklay May 30, 2018

Member

Oh, right.

@niden niden added this to In progress in 4.0 Release Nov 28, 2018

@niden niden moved this from In progress to To do in 4.0 Release Nov 28, 2018

@niden niden moved this from To do to In progress in 4.0 Release May 10, 2019

@niden niden referenced this pull request May 10, 2019

Merged

T12591 assets versioning #14056

4 of 4 tasks complete

niden added a commit that referenced this pull request May 12, 2019

niden added a commit that referenced this pull request May 12, 2019

niden added a commit that referenced this pull request May 12, 2019

niden added a commit that referenced this pull request May 12, 2019

@niden

This comment has been minimized.

Copy link
Member

commented May 12, 2019

Resolved in #14056

@niden niden closed this May 12, 2019

@niden niden moved this from In progress to Done in 4.0 Release May 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.