saguaroib / saguaro

saguaro imgboard software
14 stars 5 forks source link

New Regist #196

Closed RePod closed 8 years ago

RePod commented 8 years ago

The end to our suffering

99% safe to merge, although quality of life features (caps/admin/etc.) are not added yet. Tinfoil conspiracy theories: 1%.


Key advantages to the new Regist (as they come):


Things @Apogate needs to do:


Remove old_* files before merging.

Apogate commented 8 years ago

this gives me reason to live

RePod commented 8 years ago

This is also the last largest collection of legacy code (barring Log which is in limbo and small misc files). Following that the entire project will have been converted to the glorious Object Oriented Master Race.

Also with how things are going, this will be able to (potentially) usher in more competitive features like multi-file posts.

Apogate commented 8 years ago

multi file posting

my true goal is to find a way to make that look nicer than how its implemented on other sites

RePod commented 8 years ago

I doubt I can do much more given my limited understanding of Regist, this is where you come in @Apogate.

At the moment the dynamic query generation does not accurately replicate now and root. However, I doubt these two would stop it from being valid but I also cannot get Log to generate it.

Text only replies are inserted fine (and work when manually updated) yet have File Deleted. This may be due to the image/thumb dimensions and size being returned as 0. These should remain 0 and the sources (post generation) should be updated.

I have updated the initial message to reflect things for you to accomplish.

Apogate commented 8 years ago

With that checklist, I'm good to go.

Where has class-regist been all my life.

RePod commented 8 years ago

MLG PROTIP: When in doubt, look at the cached info which is available in Regist->cache (private).

Basically stick var_dump($this->cache) anywhere in Regist.

RePod commented 8 years ago

Also I'd eventually like to remove extract($_POST); from imgboard.php since it seems it was used primarily for the old Regist and a few edge cases overall.

You may notice this new Regist does not leverage the "power" (read: quick and easy) of something so insecure as extracting all user-generated input into the current scope (or in this case, global). Instead, it picks and chooses exactly what it needs from $_POST (and $_FILES).

A great security boost (when the extract() is removed). However, that is across the entire software. We can start now with Regist. Ensure moving forward you don't rely on extracted POST variables and instead fetch them directly.

All other extract() calls from user-controlled sources are definitely up on the chopping block. Other uses like on SQL rows is "okay", but we should be fetching an associative array from the start.

RePod commented 8 years ago

As of 172ad0c23dbde8f53135645b5e20066b8739223e, root is no longer. I briefly described some things in the commit but will now expand here.

Previously, root was being used to determine bump order by storing the timestamp of OPs but not replies. Now, modified (its replacement) stores the timestamp whenever an initial INSERT query (creating) or UPDATE (modifying) happens on any row.

It is still possible (at least via SQL, not Log?) to determine the true bump order by selecting rows with a resto of 0 and sorting by modified, similar to the old behavior.

However, another new column was introduced: last. By default, a new OP will have a last of itself to assist in determining bump order. This column is passively updated (via Regist) with the latest reply's post number.


These changes have broke legacy support:

It is possible to update older tables, but _I_ will not be doing that.

RePod commented 8 years ago

Worth noting, autonoko is extremely fast (immediately forcing a redirect when it's hit) and therefore may not catch the updated page (or may still be waiting on other caches).

This is a minor issue as users need to manually refresh the page for now anyway during actual conversation.

The last basic (working) functionality missing from Regist is updating index pages.

RePod commented 8 years ago

As of bd2b9d52c8eb7dfa894d0479de580c88c5415766, NeoRegist™ is functionally similar to the old Regist under the hood. You can post images and videos as a reply or an opening post and the server gets all the same information like old Regist, except for now and the new last/modified columns.

All that's left is @Apogate's checklist (which has been updated) and it'll be good to merge (without testing!).

The PR's initial post has been cleaned up and condensed.

Apogate commented 8 years ago

Additional issues with the deletion class have been found that probably depended on old SQL values. Might be a good time for me to revisit the deletion code like i've been meaning to. Could be condensed and rewritten.

image

I updated the original comment to reflect this.

Apogate commented 8 years ago

Going to experiment shuffling prune old into the log

Apogate commented 8 years ago

The ban system is still absolute garbage as well.

Apogate commented 8 years ago

PR 196: REWRITE SAGUARO

RePod commented 8 years ago

Yeah, I really don't like the current saguaro. I'll make a new branch that starts from scratch then submit a PR.

RePod commented 8 years ago

Ban handling in the Error class has been disabled. Also S_BADHOST in en-us.php is empty which was also part of the problem (along with bad handling code that had no reason to be in Error).

This should be handled by the Ban(ish) class.


Might as well completely document the problem (and why they're cropping up so often now).

The cooldown check in regist/process/upload.php (which makes sure we're good to start processing the post) was actually returning true (success) and incorrectly being handled therefore calling Error with empty arguments. Fortunately this exposed another problem.

One of these arguments is the error message to display, which was empty. However, the Error class had ban handling code in it that compared the received message with S_BADHOST (the banned string). If the two matched, it would redirect to banned.php and be done with it. The reason this was happening though was because at this point, Error received the empty error message and then compared it to S_BADHOST (which was also an empty string!). Therefore, a non-existent error was being thrown during the upload check and Error was vaguely comparing two strings to determine if a ban actually existed which is not only wrong but also poor form.

The Ban class should determine what happens if a ban is detected, not the Error class.

RePod commented 8 years ago

As for pruning, I'd recommend working out post and thread deletion first then having prune call the Delete class accordingly (which it's kind of already doing).

It's beyond Regist's scope (creating posts) so it's best relocated into/around Log (caching) or Delete (removing posts). I prefer Delete. Ultimately Log could handle calling the pruning after Regist calls it to update the cache.

RePod commented 8 years ago

So why exactly do we need wordwrap?

According to this search, nothing in the repository uses it (or hasn't used it in a long time) and we should be able to replicate whatever it's doing purely via CSS (including ellipsis: ...).

Apogate commented 8 years ago

Wordwrap serves an incredibly vital function that is entirely unknown to me. I'll do some testing on the old regist builds later tonight and see what it actually does, because none of that code makes sense to me anyway.

Something about explosions and line breaks. Sounds important

¯_(ツ)_/¯

Pls don't delete just yet

RePod commented 8 years ago

imgboard.php commit history.

Doesn't seem there was an actual explanation.

Apogate commented 8 years ago

but what does it DO?

RePod commented 8 years ago

Whatever it does, as stated before, it doesn't do it now and hasn't for a long time. #81 Removing/writing old and unused code makes the conversion process a lot easier, like my Fortune commit.

Apogate commented 8 years ago

I've narrowed it down to one of two things, either it handles post wrapping around images or it deals with line truncation for large comments

RePod commented 8 years ago

Both situations are best suited to be solved by CSS, the latter even more so.

As for the post wrapping around images, that's a problem with the HTML generation that would need to be addressed.

Apogate commented 8 years ago

untitled saguaro_development_cycle.jpg

Yushe commented 8 years ago

sigh

. RES_DIR .

https://github.com/spootTheLousy/saguaro/blob/new-regist/_core/regist/regist.php#L61

RePod commented 8 years ago

Don't blame me, I just copypasted code.

Also, they're lucky to even be redirected to the duplicate image. Dang millennials.

RePod commented 8 years ago

RENZOKU3 does not exist in the config.

Unused alternative to the cooldown checks, works at random and am too lazy to debug it. I also have nowhere to put it so here it is. The idea was to pull all suspicious rows with a single query.

function cooldown($upfile) {
        global $mysql;

        $resto = (int) $_POST['resto'];
        $host = $_SERVER['REMOTE_ADDR'];
        $time = time();

        $canFlood = (valid('moderator')) ? true : false;
        if ($canFlood) return true;

        //Pull all recent rows (to the highest timeout) from the SQL table.
        $min = $time - max(RENZOKU,RENZOKU2/*,RENZOKU3*/);
        $query = "SELECT time,resto FROM `".SQLLOG."` WHERE host='".$mysql->escape_string($host)."' AND time>=$min";
        $query = $mysql->query($query);
        $result = $mysql->result($query);
        $recent = $mysql->num_rows($query);

        if ($recent > 0) { //We have at least one post that violates the cooldown periods.
            //We could theoretically stop here if we don't care to give a SPECIFIC error message or care about the differences.
            $this->last = S_RENZOKU;

            //Check each row.
            while ($row = $mysql->fetch_assoc($query)) {
                //Replies (in general).
                if ($resto > 0 && $row['time'] > ($time - RENZOKU))
                    $this->last = 'reply';
                    return false;

                //Image replies.
                if ($upfile && $row['time'] > ($time - RENZOKU2))
                    $this->last = 'image '.S_RENZOKU2;
                    return false;

                //Thread creation. If no parent specified, and a pulled row is an OP (no parent): exit.
                if (!$resto || $resto == 0 && $row['resto'] == 0 && $row['time'] > ($time - RENZOKU)) {
                    //$this->last = S_RENZOKU3;
                    $this->last = 'thread';
                    return false;
                }
            }
        }

        return true;
    }

I don't particularly recommend it, and it doesn't really have any advantage over the old code. The old code just looked weird to me.

Apogate commented 8 years ago

I'll sift through and debug this tonight then. I've changed my mind and am for getting this merged as soon as we have basic functionality.

It'll give me the opportunity to optimize old features as well as add new ones

RePod commented 8 years ago

There are only a few "huge" functional shortcomings this has currently:

Fortunately, the first two can be taken care of easily in Regist->extractForm(). Third is also easy, just in another file. Alternatively we can mimic old behavior for non-existent file info. Other than that, we have very high parity with old school saguaro and can slowly add the rest back.

Apogate commented 8 years ago

5bdf81c in action:

image

ego tripping at every level now

Yushe commented 8 years ago

So do manager capcodes and permissions work now?

Apogate commented 8 years ago

If manager is the highest permission they have in the database, yes

Apogate commented 8 years ago

image

dice in action

Apogate commented 8 years ago

@RePod the cooldown works, IDs and all the fancy features are ready to go.

Stickies and locked threads are all that's left to take a look at. The flags can be set, but since we can't modify modified anymore (I believe it's set automatically), we need a new way to organize stickies.

Once that's done, regist 2 is complete.

RePod commented 8 years ago

Possibly chmod certain files per #201.

While we aren't responsible to do so, I will say it may be the server owner/provider's fault that files get certain permissions by default.

Also I believe it is faster to stat the target for its permissions (since it's a read, and can almost never fail) instead/before writing them blindly which may fail. See test.php's directory creating for an example.

The only problem with test.php's way is it uses substring, but it's better than nothing. Also notice clearstatcache() is in every loop. It'd be best to add this now so it's there in the future when multi-file uploads are made.

Resources:

Apogate commented 8 years ago

Images need to be chmod'd after creation, I had the same problem with a different host.