[ticket/14257] Reparse after update#4005
Conversation
PHPBB3-14257
PHPBB3-14257
PHPBB3-14257
|
I don't think we really need cron tasks for the forum data, contact admin info, and group descriptions. We should be able to reparse these from a single migration. The contact admin info is just a single config entry for example. |
|
Indeed, I didn't thought about that. ^^ |
There was a problem hiding this comment.
Is that conditional correct? Looks like it should not be negated.
Either way, I think it would be better to simply have a conditional with only one branch that throws an exception and no else, e.g..
if (!$this->reparse_lock->acquire())
{
throw new Exception;
}
// ...the rest executes as usualThere was a problem hiding this comment.
Argh, thanks. That's the way I wanted to write it first, but then I changed it (mainly to be consistent with everything else that acquires a lock) and forgot to change the conditional too. :/
|
What do you think of moving the loading/saving of resume data into its own class? Perhaps even the locking mechanism? Not sure about that one. Maybe it could have a simplified API to schedule reparsing via the cron. It would be useful for extension authors I guess. You could have something like that: namespace phpbb\textreparser;
class manager
{
public function get_resume_data($name);
public function update_resume_data($name, $min, $current, $size);
public function schedule($name);
} |
|
Well, that would reduce code duplication, so I guess we can do that. The locking mechanism already reuses the phpbb\lock\db class. Also why simplifying it further? I think the Extension authors actually don't have to write any class for this. For the cronjob they can just reuse this one and change the call in the service definition, while for the CLI command adding the tag to their reparser class is enough. More concerning should be that an administrator can do weird stuff in the console command which can have unexpected results for the cron job, right? What do you think about that? |
Elsensee
changed the title
|
What kind of weird stuff can they do? Reparse a single post and interrupt the cron maybe? I don't know. I think that people who use CLI commands should know better, and if they interrupt the cron (by overwriting the resume data) they'll be able to restart reparsing. I don't have a strong opinion. |
Exactly. |
PHPBB3-14257
|
@JoshyPHP Writing the manager resume_data_manager right now. What's the schedule() supposed to do actually? |
b3aa324 to
33500fd
Compare
|
|
PHPBB3-14257
PHPBB3-14257
|
Added the text_reparser manager now. (I also fixed stuff so console command and cron task now can really work together ^^) |
|
Why would you remove these options from the CLI command? This command isn't meant only for the update, it shouldn't be a command you can use only once in the life of your forum but anytime you need (or think you need) to reparse the posts (or a sub set) of your forum. The console is meant to be used by advanced user, please keep it flexible |
There was a problem hiding this comment.
This is a very technical message, you should explain what the issue is (ie. an other process might already ben using the reparser)
PHPBB3-14257
2a1bf6a to
900ccd7
Compare
|
@phpbb/native ^ |
6f17622 to
8f42541
Compare
PHPBB3-14257
0bc3bb1 to
762b383
Compare
There was a problem hiding this comment.
How about Another process is already running reparsing. or Another process is already reparsing.?
There was a problem hiding this comment.
Another reparsing process is running ?
There was a problem hiding this comment.
Another reparsing process is already running?
|
👍 |
[ticket/14257] Reparse after update * Elsensee/ticket/14257: [ticket/14257] Add tests for reparser manager [ticket/14257] Fix CLI reparser and set cron interval [ticket/14257] Fix CLI error message [ticket/14257] Fix phpdoc in CLI command [ticket/14257] Add text_reparser manager [ticket/14257] Use migrations instead of cron job for some reparsers [ticket/14257] Fix lock acquire in CLI command [ticket/14257] Fix if condition [ticket/14257] Add reparse_lock to CLI command [ticket/14257] Add cron tasks for reparsing text
7 participants
One new cron task for each reparser.
Advantage of this approach is that the decision which reparser will run in the cron job is made before running it.
Disadvantage is that extension authors have to create their own cron task but since they only have to define that in the services.yml and they have to create their own reparser class anyway, I don't think this is a big issue.
So basically no disadvantages. 😀
Cron job and console command can't interfere with eachother because the reparser acquires a lock, of course.
However, cron job and console command will be synced. So the console command can resume where the cron job stopped and vice versa.
This also means unfortunately that an administator can set weird values by using the console command (weird start, end, size IDs) which might lead to an unfinished reparsing. I don't know if this is something we have to care about?
And since this is your baby, @JoshyPHP, please have a look as well. :) ;) Thank you! :)
PHPBB3-14257