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

[WIP] Attachments #3580

Open
wants to merge 5 commits into
base: development
Choose a base branch
from
Open

Conversation

emanuele45
Copy link
Contributor

It was a while I wanted to kill again the attachments.
That's really just a bit of makeup to warm me up. The Attachment class is a placeholder, I just wanted to put these constants somewhere because it always bothered me I could never find what the heck the numbers were. It could hold much more, worst case a good chunk of Attachments.subs.php in a static way.

I was thinking of a couple of things:

  1. drop the storing of the thumbnails in the db (just rely on a prefix to the file name and GTFO),
  2. maybe move the uploaded avatars out of the way (that as far as I remember is a good chunk of annoying code to deal with),
  3. if not too messy, maybe go back to when there was one way to store attachments, just not a single directory, I'd say by months is usually good enough and easy to deal with?

What do you think @Spuds ?

@emanuele45 emanuele45 added this to the 2.0 milestone Jan 31, 2022
@Spuds
Copy link
Contributor

Spuds commented Feb 1, 2022

WooHoo More attachment fun 👍

  1. Not storing the thumbs in the db ... I feel this makes sense. I think the code still does a fileexists after the db call so why not just look for the attachment hash + _thumb. Should save some overhead I think.

  2. I abused the avatar code in a recent commit (no more uploads as attachments, removing all those db calls). Moved all of the avatar upload code out of profile into its own class, and more so just unraveling that messy code. It really did not do what it claimed to do. Anyway any improvements there are welcome.

  3. You mean drop the auto directory options and just make it year/month going forward. That should simplify several areas. I feel some of the current options are a bit confusing and make the UI difficult to follow.

@emanuele45
Copy link
Contributor Author

Dealing with attachments is more of an headache than I remembered. 😆

  1. yep, apparently they are already saved with "thumb" but stil lalso in the db with the whole id thingy... confusing.
  2. I saw, didn't notice at first!
  3. yep, I wanted to do it a while ago but gave up. The only downside is that we'd have to reprocess all the attachments during the upgrade... that may become a bit time consuming...

@codecov-commenter
Copy link

Codecov Report

Merging #3580 (6bc508f) into development (d3bbbb4) will decrease coverage by 4.13%.
The diff coverage is 0.00%.

❗ Current head 6bc508f differs from pull request most recent head 2dcbd0d. Consider uploading reports for the commit 2dcbd0d to get more accurate results

@@                Coverage Diff                @@
##             development    #3580      +/-   ##
=================================================
- Coverage          30.52%   26.39%   -4.14%     
+ Complexity         15723    15534     -189     
=================================================
  Files                423      439      +16     
  Lines              74098    75286    +1188     
=================================================
- Hits               22616    19868    -2748     
- Misses             51482    55418    +3936     
Impacted Files Coverage Δ
sources/ElkArte/Attachments/Download.php 0.00% <0.00%> (ø)
sources/ElkArte/Controller/Attachment.php 26.78% <0.00%> (+16.56%) ⬆️
sources/subs/Attachments.subs.php 15.36% <ø> (-2.24%) ⬇️
sources/ElkArte/Errors.php 0.00% <0.00%> (-100.00%) ⬇️
sources/ElkArte/Modules/Mentions/Post.php 0.00% <0.00%> (-100.00%) ⬇️
sources/ElkArte/Modules/Mentions/Display.php 0.00% <0.00%> (-100.00%) ⬇️
sources/ElkArte/Notifiers/AbstractNotifier.php 0.00% <0.00%> (-100.00%) ⬇️
...urces/ElkArte/UrlGenerator/Standard/ParseQuery.php 0.00% <0.00%> (-100.00%) ⬇️
...rces/ElkArte/ScheduledTasks/Tasks/WeeklyDigest.php 0.00% <0.00%> (-100.00%) ⬇️
...rces/ElkArte/Modules/Mentions/AbstractMentions.php 0.00% <0.00%> (-90.91%) ⬇️
... and 352 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 773df0f...2dcbd0d. Read the comment docs.

@tenbuddy

This comment was marked as spam.

@@ -286,8 +286,7 @@ public function action_dlattach()
// We need to do some work on attachments and avatars.
require_once(SUBSDIR . '/Attachments.subs.php');

$id_attach = $this->_req->query->attach ?? '';
$attachment_class = new Download($id_attach, $this->_req->getQuery('attach', 'intval', 0));
$attachment_class = new Download($this->_req->query->attach ?? '');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$attachment_class = new Download($this->_req->query->attach ?? '');
$attachment_class = new Download($this->_req->query->attach ?? 0);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants