craftcms / cms

Build bespoke content experiences with Craft.
https://craftcms.com
Other
3.21k stars 616 forks source link

File volume Asset issues apparently on beta 5 #1437

Closed narration-sd closed 7 years ago

narration-sd commented 7 years ago

Description

@andris-sevcenko Andris, I'm moving discussion on these from #1432 as requested. Apologies for ending up piling in there -- original intent was support, but as you note I ran into other things.

In summary, since details are in #1432,

Steps to reproduce

  1. composer update or install to Craft 3.0 beta5
  2. have some images in an Assets volume (defined volumes.php); volume is arranged in Settings/Assets
  3. run a Clear Caches and an Update Asset Index in Utilities
  4. note that the image set directory structure now shows in main menu Assets, but that there are no assets themselves there.
  5. Attempts to access the images in Twig fail, though direct url access to the images folder path will transfer without issues and show the images fine in web browser
  6. As experiment, use the CP to upload and add a fresh image to the same Volume. This proceeds apparently well, and this image does show in the Assets folder where it should.
  7. After Clear Caches and Update Asset Index, this new file is accessible from Twig -- but, transform versions are neither created in the filesystem for it, nor naturally then web accessible.
  8. Looking at the database tables involved fits the situation; xdebug trace seems reasonable up to the add-transform Task creation; didn't explore beyond that. Some further details in #1432, and I don't think they've changed with your update.

[a note and extra points question: I had a lot of trouble to install your patch -- of course Composer is not going to see it as version number doesn't change. I finally ended up pulling my separate clone of the cms repository, and manually pulling in your change via PhpStorm diff. Is there a better way, as this would be hairy with a batch of file changes? I leave it to your imagination what I did to my /vendor area before working this out...we always have to learn once, understand better after..cheers in Riga....]

Additional info

PHP version 7.1.2-3+deb.sury.org~xenial+1 Database driver & version MySQL 5.7.17-0ubuntu0.16.04.1 Image driver & version GD 7.1.2-3+deb.sury.org~xenial+1 Craft edition & version Craft Pro 3.0.0-beta.5 Yii version 2.0.11.2 Twig version 2.1.0 Guzzle version 6.2.1 Imagine version 0.7-dev Plugins - none

This is all running in a fresh Vagrant, but the same transpires on another well-experienced install running natively on a Ubuntu 16.04 up-to-date, in its own vm. The Vagrant is also running 16.04.

narration-sd commented 7 years ago

Ok, it must be [news at] 11 - another clue.

Now, I suspect by your one-liner change, there is a record in assettransformindex for the newly registered transform. However, it's flawed -- it doesn't mention the file applied to (just the one file at present).

Pix attached to show this state, plus the look of the assetransformindex records on a working beta4.

transform-index-without-filename

assetindex-normal-records

narration-sd commented 7 years ago

Just to liven things up, for that is where we actually live, here's a screenshot showing the image that should be there, the blank image marker where the transformed version should be, and above them, the make-a-transformed url presented for it.

image-and-nonlocation

The woman, but her music, as a sidenote... Giving you two, in case the subject matter in first is too far, but I think you can well take it :) -- esp. as presented. This is out of the real folk tradition of saying things; used to sing it. And you can see how she inspires those who play with her.

https://www.youtube.com/watch?v=UuXb4She_sU https://www.youtube.com/watch?v=5WtuDIb2f0Y

Goes with, and in fact they play together these days in a trio sometimes, part of performing:

https://www.youtube.com/watch?v=X09s37tJ09s

narration-sd commented 7 years ago

As far as a better method for pulling in Github interim updates , I found this and discussion, with the right Google key this time. Should solve the need, unless you guys know better.

https://packagist.org/packages/cweagans/composer-patches https://pantheon.io/blog/patching-and-composer-workflow (great notes)

And it does - works a treat, gets everything, writes a note per dependency area saying what it did. Do need to run composer update from Gitbash or equivalent to succeed on Win, so patcher can find a /tmp folder.

andris-sevcenko commented 7 years ago

Indexing is definitely busted at the moment. Looking into that.

andris-sevcenko commented 7 years ago

Note: This actually, again, contained two issues. Working on transform issue now. Will ping back when done.

andris-sevcenko commented 7 years ago

Okay so if this is not fixed, at least there's some definite headway here. I got blindsided by a validation related commit that exposed a larger Asset indexing logic issue.

Fixed that and hopefully all your problems (disclaimer: only problems in this issue are referred) will be solved. If not, well, re-open and let's dance again :)

narration-sd commented 7 years ago

Ok, just want to let you know ‘I’ll be back’ on this later, since it’s passing closing time in Riga. Something else needs to get done this afternoon first.

I’m held up because my new handy-dandy patch manager refuses to load your first patch of today’s pair. With no helpful handy-dandy error message except the refusal…

It does seem you are on the case, though, and I anticipate good things, thank you kindly. New perhaps at 11.

Ok, and will withhold the entertainment in future, as apparently Craft is too busy for such. I just thought in this case you and Dace might particularly appreciate the set. Maybe later, or on another channel.

Why does fooling with version control so often feel like thrashing?? Rhetorical question to be sure.

(and yes, I forgot to reply all in the mail version of this, so you probably get a second notice from Github)

narration-sd commented 7 years ago

Ok. I can't see how to re-Open, probably because I'm not permitted to here.

Yes, matters are improved.

However:

asset-transform-index-no-filenames

Also:

Ok. I feel like I really revisit(?) historic devland, as I wipe whole installations etc... It's the only way sometimes even yet. I spare you my new and interesting test photos, of sparkling Svolvaer harbor, of a scene from Syberia 3. I will report elsewhere when I get to it that versioning and live preview don't seem to be working yet at least on Singles. I bid you adieu...;)

andris-sevcenko commented 7 years ago

here and on a beta4 I've left untouched, the assetindexdata table remains empty, yes after indexing. Presumably this has consequences if there are a lot of them.

assetindexdata table gets pruned after indexing as that table is only used during indexing.

Image titles on the prior-present assets weren't being picked up, instead 'New Asset' was used on all of them.

You mean existing files? Assets are Craft entities and files are the files that the Assets represent. For existing files the wrong title generation was fixed with 1a59022

after a while, I lost the ability to show any Section fields in twig pages.

Doesn't seem related and I wouldn't be the right tree to be barking up at ;)

How I resolved the patches thing. Composer -vvv gave some highly entertaining and not quite right messages

Have you considered just cloning the git repo? :)

narration-sd commented 7 years ago

Ach. I thought answering your email with Reply All would post here also. Apparently not. Forthwith answers then properly.

Clearer formatting here than email makes likely, and also I realized a few things in the interim:

assetindexdata table gets pruned after indexing as that table is only used during indexing.

Fair enough; that one off my list. I thought to chide you for not using a temp table, but reckoning the nature of web apps (idempotency, Craft's task arrangements, etc.) I realized probably why you have this stable table instead.

Image titles on the prior-present assets weren't being picked up, instead 'New Asset' was used on all of them.

You mean existing files? Assets are Craft entities and files are the files that the Assets represent. For existing files the wrong title generation was fixed with 1a59022

Ok, that’s why it's proved to work now, as I'd picked up that commit’s patch

after a while, I lost the ability to show any Section fields in twig pages.

Doesn't seem related and I wouldn't be the right tree to be barking up at ;)

I agree, off the list. It’s not impossible that something was getting a little unstable in this build, but it will show up again if real, and I’ve killed and replaced the vm anyway. Built-from-scratch was fine. It was a little weird while it lasted.

How I resolved the patches thing. Composer -vvv gave some highly entertaining and not quite right messages

Have you considered just cloning the git repo? :)

Of course – I mentioned yhe idea to you, and anyway I have an inactive clone of both dev and master. But would prefer to run off what you release plus necessary patches, and anyway was proving out and usual detail understanding development of the patching mechanism for composer.

Thinking of reconsidering, though, as you guys seem to keep a pretty sanitary dev (?!) – and it would be nice not to be encountering or reporting flaws already smoothed.


Ok – I think we are down to the last point on this issue: that the proper transformed assets not getting physically generated, so I can't yet use them. That there is no filename for them in the assettransformindex table (as in screenshot above in prior messsage) is either a symptom or a cause. . And just to close in on a mystery also reported, I think I may have been causing the difficulties with transformed images not showing up on vm startup without clearing cache and rebuilding the asset index. I use rsync a lot whether on Docker or Vagrant vms, and discovered chasing other problems today that it's going to take some changes in options to fit properly with the new C3 file layout. Said rsync options can also be rather tricky, especially as some of them interact., as you probably know. I'll let you know when I've got that properly handled, but really suspect Craft beta is off the hook here..

andris-sevcenko commented 7 years ago

I can't reproduce this on my end - if I debug I can catch a moment where filename is null in assettransformindex but, by the time the page finishes rendering, it's filled. I'll patch it, anyway, since it never should be null, but I doesn't seem to be the culprit here.

Can you start off with reindexing Assets and clearing transform index? Or letting me know if you can reproduce this on clean install.

andris-sevcenko commented 7 years ago

Actually, scratch my remark about patching the filename being null. That's how it should be, since the filename is the final filename for transform and Craft cannot know that until it generates the transform. This is for cases when you have a set the transform format to "auto" and are uploading, for example, a tiff file. Depending on whether or not it has a transparency channel the transform can end up being a jpg or a png file, so the transform filename is not know until Craft tries to generate the transform when it actually makes the decision about the final filename (if it has to.)

So, we're back to "reindex, clear transform index, use care when migrating between installs" and "can you reproduce on clean install?".

narration-sd commented 7 years ago

Ok, that insight gives the order to events I was looking for.

These experiments have all been done on fresh install, with only that simple page I showed you, but I will do it again from scratch to be sure, no problem.

Tied up at moment but later today, thanks, Andris.

narration-sd commented 7 years ago

Ok, Andris. I have finally nailed this, and with a picture to prove below, two versions of lady this time, properly according to a transform.

I did two things (you can imagine the time and details):

Voila. Now finally the image transform worked. !!

I'm not sure what to call a flaw here. Is it something in your code, that it reacts badly to the internal notion that the site url is false? Or is it a fault in Craft 3's handling of the site name definition? Or would safety try to argue both, though fixing that may be impractical?

What caused the doubling-false internal site url value is this.

That second point was the mistake. C3 added the two strings together, in its very own way, and your code for transforms at least didn't like it, refused to build the actual transform file, while not throwing an exception of a kind that would reach the browser screen level.

I suspect it should throw that exception. And while arranging that, it would be good to look at the arrangments for the exception thrown if the transform name is incorrect in the Twig asset.getUrl/.url('name') of a template. I used that error to provoke towards solution trace here, and found it put a 500 small-alert error on the browser screen even when devMode was set, not a stack trace. So the only way you could find that was in the logs -- or with the great Yii2/Craft-extended debug toollbar.

In any case, I fixed matters, with the results you see, by simply commenting out the siteUrl definition in /.env.

A lot of text. I hope this level of detail is suitable, and appreciate much your sticking with me on this to get to this solution point. It works. You know the feeling.

Now, a bonus I think, and I will put that in a new issue, the mystery mentioned before.

Meanwhile, here is the lady, two proper views:

two-aoife

narration-sd commented 7 years ago

And well, solved even better, so that the 'mystery of the transforms disappearing across server regenerations' is gone, and was not on your guys' table at all.

This one was confused by the main issue just solved, because transforms were never generated until your patches plus the problem found which should have its own patches was cleared.

Transforms might be brought up from past generations in the sources (actually improperly), but weren't addressed for a Twig page because they hadn't been created and marked in database by your code. Or found anyway, due to the last problem just above.

What was really going on underneath all of it was one more scringely detail in the way rsync exclude definitions s actually work. That repaired, no more mystery, ladies always showing. Other transforms in other more demanding sites just as well.

You are welcome, and looking forward to the delivery of your treatise in the coming Hangout.

Cheers, Andris

andris-sevcenko commented 7 years ago

In that case if your generateTransformsBeforePageLoad was set to false, the whole transform process would be set up when rendering template, filling in the craft_assettransformindex table with the transforms needed to be generated. And afterwards, Craft could not call home to actually generate those transforms because of the faulty site URL.

Maybe. Or not. Admittedly, I've only had one coffee yet, so I might be wrong.

Either way, closing this and leaving this one behind us :)

narration-sd commented 7 years ago

amen ;)

narration-sd commented 7 years ago

I'm writing in the faulty site url bug - will fill in the # here