Added assets versioning #12591

Jurigag · 2017-02-01T17:31:55Z

Hello!

Type: new feature
Link to issue: [NFR] Implement versioning in Assets Manager #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 · 2017-02-01T17:38:37Z

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

sergeyklay · 2017-02-01T19:02:47Z

phalcon/assets/manager.zep

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


I dislike such pasta code

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

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

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.

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

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.

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

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

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?

sergeyklay · 2017-02-01T19:03:00Z

phalcon/assets/manager.zep

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


The same here

david-duncan · 2017-02-01T23:57:02Z

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

Jurigag · 2017-02-02T00:26:17Z

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 · 2017-02-02T11:30:13Z

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

Jurigag · 2017-02-02T11:33:26Z

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.

sergeyklay · 2017-02-02T12:14:06Z

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 · 2017-02-02T12:15:35Z

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 · 2017-02-02T12:38:09Z

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

sergeyklay · 2017-02-02T12:40:31Z

Do you really like such design?

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

Jurigag · 2017-02-02T14:03:04Z

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 · 2017-02-02T14:07:04Z

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 · 2017-02-02T16:43:01Z

Failing tests seems unrelated.

Jurigag · 2018-05-28T16:45:12Z

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

sergeyklay · 2018-05-28T19:32:44Z

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

Jurigag · 2018-05-30T06:48:00Z

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

sergeyklay · 2018-05-30T16:58:28Z

@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 · 2018-05-30T17:01:03Z

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

sergeyklay · 2018-05-30T17:02:53Z

phalcon/assets/collection.zep

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


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

sergeyklay · 2018-05-30T17:07:24Z

phalcon/assets/resource.zep

 	/**
 	 * 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)


@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

sergeyklay · 2018-05-30T17:08:47Z

phalcon/assets/resource.zep

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


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

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

niden · 2019-05-12T20:31:07Z

Resolved in #14056

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

Jurigag force-pushed the 3.1.x-assets-versioning branch from 8bf8fb3 to 357d09e Compare February 1, 2017 17:51

sergeyklay suggested changes Feb 1, 2017

View reviewed changes

Jurigag force-pushed the 3.1.x-assets-versioning branch 2 times, most recently from b3576ee to 5428fdb Compare February 2, 2017 12:09

Jurigag force-pushed the 3.1.x-assets-versioning branch from 5428fdb to 57d502e Compare February 2, 2017 14:01

Jurigag force-pushed the 3.1.x-assets-versioning branch 2 times, most recently from 70647a0 to d2b7d2b Compare February 2, 2017 15:04

Jurigag closed this Feb 2, 2017

Jurigag reopened this Feb 2, 2017

sergeyklay added the New Feature Request label Mar 8, 2017

sergeyklay added this to the 3.2.0 milestone Mar 8, 2017

sergeyklay force-pushed the 3.1.x branch from 928502b to b0a7493 Compare March 9, 2017 14:19

sergeyklay force-pushed the 3.1.x branch from f115731 to c7de98d Compare March 21, 2017 23:32

Jurigag changed the base branch from 3.1.x to 3.2.x March 23, 2017 11:06

Jurigag force-pushed the 3.1.x-assets-versioning branch from d2b7d2b to deb303e Compare March 24, 2017 14:17

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

Added assets versioning

3364d85

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

Jurigag changed the base branch from 3.4.x to 4.0.x May 28, 2018 16:44

Jurigag closed this May 28, 2018

Jurigag reopened this May 28, 2018

sergeyklay closed this May 30, 2018

sergeyklay reopened this May 30, 2018

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

sergeyklay suggested changes May 30, 2018

View reviewed changes

niden assigned Jurigag Nov 1, 2018

niden added new feature request Planned Feature or New Feature Request and removed New Feature Request labels Mar 11, 2019

niden mentioned this pull request May 10, 2019

T12591 assets versioning #14056

Merged

4 tasks

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

[#12591] - Added assets versioning

63faab5

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

[#12591] - Merging changes

9fdf80e

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

[#12591] - Adjusted tests and minor fixes

d1d6b04

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

[#12591] - Updated changelog

1e9c562

niden closed this May 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added assets versioning #12591

Added assets versioning #12591

Jurigag commented Feb 1, 2017

Jurigag commented Feb 1, 2017 •

edited

Loading

sergeyklay Feb 1, 2017

Jurigag Feb 1, 2017

sergeyklay Feb 1, 2017

Jurigag Feb 1, 2017 •

edited

Loading

sergeyklay Feb 1, 2017 •

edited

Loading

Jurigag Feb 1, 2017 •

edited

Loading

Jurigag Feb 2, 2017

Izopi4a Feb 2, 2017 •

edited

Loading

Jurigag Feb 2, 2017 •

edited

Loading

sergeyklay Feb 1, 2017

david-duncan commented Feb 1, 2017

Jurigag commented Feb 2, 2017 •

edited

Loading

sergeyklay commented Feb 2, 2017 •

edited

Loading

Jurigag commented Feb 2, 2017 •

edited

Loading

sergeyklay commented Feb 2, 2017

Jurigag commented Feb 2, 2017 •

edited

Loading

sergeyklay commented Feb 2, 2017

sergeyklay commented Feb 2, 2017 •

edited

Loading

Jurigag commented Feb 2, 2017 •

edited

Loading

sergeyklay commented Feb 2, 2017

Jurigag commented Feb 2, 2017

Jurigag commented May 28, 2018

sergeyklay commented May 28, 2018

Jurigag commented May 30, 2018

sergeyklay commented May 30, 2018 •

edited

Loading

sergeyklay commented May 30, 2018 •

edited

Loading

sergeyklay May 30, 2018

sergeyklay May 30, 2018

sergeyklay May 30, 2018

StudioMaX May 30, 2018

sergeyklay May 30, 2018

niden commented May 12, 2019

Added assets versioning #12591

Added assets versioning #12591

Conversation

Jurigag commented Feb 1, 2017

Jurigag commented Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jurigag Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

sergeyklay Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

Jurigag Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Izopi4a Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Jurigag Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-duncan commented Feb 1, 2017

Jurigag commented Feb 2, 2017 • edited Loading

sergeyklay commented Feb 2, 2017 • edited Loading

Jurigag commented Feb 2, 2017 • edited Loading

sergeyklay commented Feb 2, 2017

Jurigag commented Feb 2, 2017 • edited Loading

sergeyklay commented Feb 2, 2017

sergeyklay commented Feb 2, 2017 • edited Loading

Jurigag commented Feb 2, 2017 • edited Loading

sergeyklay commented Feb 2, 2017

Jurigag commented Feb 2, 2017

Jurigag commented May 28, 2018

sergeyklay commented May 28, 2018

Jurigag commented May 30, 2018

sergeyklay commented May 30, 2018 • edited Loading

sergeyklay commented May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niden commented May 12, 2019

Jurigag commented Feb 1, 2017 •

edited

Loading

Jurigag Feb 1, 2017 •

edited

Loading

sergeyklay Feb 1, 2017 •

edited

Loading

Jurigag Feb 1, 2017 •

edited

Loading

Izopi4a Feb 2, 2017 •

edited

Loading

Jurigag Feb 2, 2017 •

edited

Loading

Jurigag commented Feb 2, 2017 •

edited

Loading

sergeyklay commented Feb 2, 2017 •

edited

Loading

Jurigag commented Feb 2, 2017 •

edited

Loading

Jurigag commented Feb 2, 2017 •

edited

Loading

sergeyklay commented Feb 2, 2017 •

edited

Loading

Jurigag commented Feb 2, 2017 •

edited

Loading

sergeyklay commented May 30, 2018 •

edited

Loading

sergeyklay commented May 30, 2018 •

edited

Loading