Feature subscription #733
Feature subscription #733
Conversation
|
Unfortunately, the merge is no fun. Two main problems:
|
|
Sorry, to keep it clear: are already in master via cherry-pick. What I did not know at the time is that git not necessary recognizes theses as such. So, I would propose to make a rebase of the feature_subscription branch first, that should take care of these cherry-picked commits and then most of the conflicts should be gone. To resolve the conflicts in bundled-libs, add simply /voku/html2text with its dependencies in the master, and in the rebase, i am going to delete the commits which are responsible for the conflicts in this directory. |
There was a problem hiding this comment.
First, thanks a lot for this huge amount of work! The mail subscription feature was requested multiple times, it's a very positive change and as far as I can see right now this should work fine!
This is a first code-only review. I did not test the functionality yet.
There are a couple of things that should be improved:
- There are number of commits in this PR that are unrelated to the subscription feature. I marked those I noticed. They should all be removed (most or all of them are already in master anyway), so we can focus on the subscription change. It's big enough as it is.
- There are some changes that I'd like to be separated from this, the author profile page comes to mind. It's just functionally a big change to the frontend and deserves its own discussion. There might be some features to be added, a different way to implement them, there might even be reasons not to add them. Like, don't we already have a plugin for those profiles? How will the profiles go together with the current authors archive pages?
- Some changes break backwards compatibility unnecessarily. I marked those I saw. I realize that some are at the very least hard to avoid or even advisable, like ignoring
subscribedin the comments table in favor of the new subscription table. Those are probably okay. But renaming theserendipity_mailSubscribersfunction for example realistically can't be done, same thing with the{BOOLEAN}change.
include/functions_images.inc.php
Outdated
| @@ -3242,6 +3242,7 @@ function serendipity_showMedia(&$file, &$paths, $url = '', $manage = false, $lin | |||
| 'nr_files' => count($file), | |||
| 'keywords' => explode(';', $serendipity['mediaKeywords']), | |||
| 'thumbSize' => $serendipity['thumbSize'], | |||
| 'multiselect' => isset($serendipity['GET']['multiselect']) ? serendipity_db_bool($serendipity['GET']['multiselect']) : true, | |||
There was a problem hiding this comment.
I will leave a similar comment in all those cases: Since this is already in master new it shouldn't be in the PR. If we can just ignore it after the merge and it does not show up two times, then those comments of mine can be ignored. But I suspect (it's a bit more git stuff that I'm confident with) that's something that needs to be addressed before merging.
include/admin/images.inc.php
Outdated
| @@ -17,7 +17,7 @@ | |||
| // No echo output here, before the switch, since that matters renaming alerts! | |||
|
|
|||
| // unset adminAction type to default, if an image was bulkmoved and the origin page reloaded | |||
| if (!is_array($serendipity['POST']) && $serendipity['GET']['adminAction'] == 'multidelete') { | |||
| if (!is_array($serendipity['POST']) && $serendipity['GET']['adminAction'] == 'multicheck') { | |||
There was a problem hiding this comment.
A commit that shouldn't be in the PR.
bundled-libs/autoload.php
Outdated
| @@ -2,6 +2,6 @@ | |||
|
|
|||
| // autoload.php @generated by Composer | |||
|
|
|||
| require_once __DIR__ . '/composer' . '/autoload_real.php'; | |||
| require_once __DIR__ . '/composer/autoload_real.php'; | |||
There was a problem hiding this comment.
Would be great if this commit could be removed as well, since it was done in master already.
| global $serendipity; | ||
|
|
||
| $fullCountQuery = "FROM {$serendipity['dbPrefix']}subscriptions WHERE 1"; | ||
| if (empty($filter_sql)) $filter_sql = 1; |
There was a problem hiding this comment.
Minor: Please use { } even for one liner ifs. These tricks tend to break and there was a prominent security bug (in a different software) because of an if starting like this and then lines being added.
|
|
||
| if ($_SESSION['serendipityAuthedUser'] !== true) { | ||
| serendipity_plugin_api::hook_event('frontend_subscribe', $data); | ||
| if ($data['block_subscription']) return 'block'; |
include/functions.inc.php
Outdated
| @@ -716,7 +717,7 @@ function xhtml_cleanup($html) { | |||
| function serendipity_fetchAuthor($author) { | |||
| global $serendipity; | |||
|
|
|||
| return serendipity_db_query("SELECT * FROM {$serendipity['dbPrefix']}authors WHERE " . (is_numeric($author) ? "authorid={$author};" : "username='" . serendipity_db_escape_string($author) . "';")); | |||
| return serendipity_db_query("SELECT * FROM {$serendipity['dbPrefix']}authors WHERE " . (is_numeric($author) ? "authorid={$author};" : "username='" . serendipity_db_escape_string($author) . "';"), true, 'assoc'); | |||
There was a problem hiding this comment.
Assuming the function was not broken before that looks like an API change we can't simply make. It might break plugins.
lang/langsorter.php
Outdated
| @@ -164,7 +165,7 @@ function synclang($dir, $lancode) { | |||
| echo "OK\r\n\r\n"; | |||
|
|
|||
| // process single file | |||
| if ($lancode != 'all') { | |||
| if ($lancode != '-all') { | |||
There was a problem hiding this comment.
Seems unrelated to the subscription feature to me, might already be in master.
| @@ -1,3 +1,4 @@ | |||
| 2.21.6: * add missing 'u' inline element | |||
| @@ -172,6 +173,7 @@ static function register_default_plugins() | |||
| serendipity_plugin_api::create_plugin_instance('@serendipity_plugin_archives'); | |||
| serendipity_plugin_api::create_plugin_instance('@serendipity_plugin_categories'); | |||
| serendipity_plugin_api::create_plugin_instance('@serendipity_plugin_syndication'); | |||
| serendipity_plugin_api::create_plugin_instance('@serendipity_plugin_subscribe'); | |||
There was a problem hiding this comment.
I'd be more comfortable with not making it a default plugin directly and having it one cycle as opt-in.
| @@ -1042,26 +1001,28 @@ function serendipity_saveComment($id, $commentInfo, $type = 'NORMAL', $source = | |||
| * @param string The body of the comment that has been made | |||
| * @return null | |||
| */ | |||
| function serendipity_mailSubscribers($entry_id, $poster, $posterMail, $title, $fromEmail = 'none@example.com', $cid = null, $body = null) { | |||
There was a problem hiding this comment.
We can't simply rename that function. Plugins might use it.
|
As I mentioned before, I would make the rebase as soon as the needed library is in master. That should clean up a lot of the overhead. And I would like to do that as soon as possible. I committed to the feature branch too because that is my running live copy with all the changes combined. As for the if without brackets, I fix these, same as adding the subscription sidebar plugin by default. But I think it can be activated by default because the Feed is also in default. And for the changes in several functions: My best friend is a fast SSD and EffectiveFileSearch. I am going to double-check now, but I am fairly sure that I corrected all the occurrences of the functions. I changed serendipity_fetchAuthor, serendipity_approveComment, serendipity_deleteComment, serendipity_mailSubscribers, serendipity_cancelSubscription, serendipity_addCategory; But i thoroughly checked all the places where these functions were used and double-checked again just now. It is safe. But I had to clean these up, otherwise everything will just mess up more over time if you patch one patch over another instead of reordering it the cleanest way. And I just cannot work in that manner. As I wrote on the Slack channel: to be able to send mails in a different language, the SIGNATURE is in the way. If I add a new variable to the function call, that will break the change I already made with the adding of the optional html email content. And if I stop the signature from being added automatically, I have to do so on every function call but this seems to be the better choice, even if there are 12 occurrencies. Because it is possible that the recipient has a different language and that has to be checked anyway. And the tagged translation of the Blog title might be messed up from the start because that one is stripped from the tags in genpage and not if the Title is mailed to someone. Someone commented in /admin/images.inc.php: What I can do is to split the branch into two branches: 2.4.alpha3 is the subscription feature and 2.4.alpha4 is the category and author info. But for example, the 'lang' column in the subscription table is added in alpha4 because it was not needed for alpha3, but will be needed in the multilingual version of emailing I am working on now where I don't need a database upgrade. It would take heavy rebasing to move that part one version down, and I already tested the upgrade process 2.4.alpha2 -> 2.4.alpha3 -> 2.4.alpha4 and that might accidentally break it. What do you mean with archives author page? The page which is shown by /index.php?/authors/1-xxx ? That page essentially does only show all the entries written by that author and nothing else. I just put a section in front of it with the Feed and Subscribe button, and added an author image and description. Everything is optional and by default empty, so when no content is inserted, the page is the same as before. The same is true for the category page, but there the description and icon already existed, but was not displayed on the category page. |
|
In my opinion, investing time into improving the core only makes sense if you can actually improve it. That includes changing functions. I worked with enough software and it happened often enough that after a new release a lot of the external plugins quit working until their developers implemented the necessary changes. So, I am not going to add another case to a constant which has exactly four uses in all the code which is available to me when the logical solution is the one I used, because restricting a general 'boolean' to 'not null default true' simply doesn't make sense. The same is more or less true for the other changes to functions I made: It should have been so in the first place. I would even go so far and change the approach to the additional plugins completely. For example, for the last days I got every day an email from my Joomla system that I should update the core, and also get similiar mails that I should update the plugins because otherwise their functionality is not guaranteed. I made the changes over all the plugins in the core package, also for the additional plugins, and everyone else who is capable of writing their personal plugins should be very much capable of reacting to a changed order for the variable input for example if it is properly documented (perhaps I can do that explicitly). And for everyone who is trying to write their own plugins, how cryptic is it that {BOOLEAN} means 'boolean not null default true' and there is perhaps another one for 'boolean not null default false' and if he needs boolean null default whatever, then he needs to add another one or hack it ... nope. So, I would propose to bundle all the plugins for 2.3.x into one folder of the repository and create another folder 2.4.x with the plugins purged from all the old stuff and ideally only these where their developers tested them with the 2.4 core. The 2.4.x Spartacus then simply fetches the plugins from the 2.4 folder. That is the way it is handled almost everywhere I know, bundled by releases. Universal, release-independent plugins are the total exception and you simply don't know which ones are abandoned and which aren't. Im truly sorry that I have to state that so plain, but I have invested a LOT of time since February on this project and intend to do my share of follow-up work. For my personal blog, I am already almost where I want because it has all the changes. But as stated in the beginning, my motivation depends on the possibility that I can shape it into the solution I envision (which is, by the way, something i mentioned on my blog because that is a result of my personality and enneagram type). And with that vision crumbling, the motivation crumbles, too. But I am also the type where the end result counts. To get there and make it easier for you, I researched all the git rebasing options and I can pull all these commits apart to order them strictly into feature branches, as long as nobody else worked with them. Just add the one library (because I don't know what happens if I run the composer update) and then I will do my part. And by the way: I truly think that s9y is "the best blog around", but I am afraid that its slice of the market is much thinner than the big ones like Blogger or Wordpress. The only major feature what I think is missing in the core are fixed assigned pictures for entries (normal and small version), that is something you see very often in themes for the ones above. Which would be easy to implement, too. But I think at least part of that is caused by the not-so-shiny front door: Someone who wants to start a blog would certainly look at the designs available and while the other two - or Joomla which is too complicated for a simple blog - have hundreds to thousands of templates, the number for s9y made in the last ten years is quite … small. Do you have any data about market share and actively attended blogs with s9y? While there are 38 languages total, the only fully translated ones are german and english … And by the way … I just saw that I did implement a somewhat much simplefied version of the userprofiles plugin with just description and picture. But it doesn't cause a collision because if there is no content, my version hides automatically and doesn't interfere. But I needed a place outside of the authors and categories sidebar to put the subscribe and feed buttons, now you can hide them on the sidebars and only display them on the category and author pages which looks much nicer in my opinon. But you can simply configure that how you like. |
|
Hi Stephan |
stephanbrunker
force-pushed
the
feature_subscription
branch
6 times, most recently
from
18166b5
to
062cc4a
Compare
stephanbrunker
force-pushed
the
feature_subscription
branch
from
062cc4a
to
6add1d3
Compare
stephanbrunker
force-pushed
the
feature_subscription
branch
2 times, most recently
from
0702d3b
to
280013e
Compare
stephanbrunker
force-pushed
the
feature_subscription
branch
from
280013e
to
cee3b3e
Compare
|
Hi @stephanbrunker. So I now finally had time to test this. It does not work in my dev blog, I think because the upgrader(?) relies on There is one The solution here is usuallly to not use the ALTER TABLE command, but insted to create new tables and move the data over. In this case I assume the ALTER TABLEs are not necessary: The DROP COLUMN can be dropped, the key can be set during table creation. The upgrader also throws an error on the I still think this feature should be implemented with less significant changes to the core, this upgrader issues are a good example of the kind of issues we'd run into with this otherwise. But I'd be happy to hear from @th-h and @garvinhicking what they think, specifically about the review comments made above, #733 (review). |
|
That looks to be a specific problem only related to the SQLite version of the database upgrader. Because I really don't have any experience with SQLite, I just tried to hammer something together similar to existing stuff (hence the sql upgrade files) without the ability to test. So perhaps there are others with more knowledge in that environment able to fix these syntax problems and are of course better qualified to make the changes? The goal is well defined, it simply should do the same as the MySQL version which works fine as intended. (Btw, the same is true for the PostgreSQL version, which I also couldn't test). Because I don't have either an SQLite environment nor a database with actual data, I have to pass this one. Then it just condenses down into the question if the feature is worth having others do their contribution (which is marginal compared to my effort) or not, I did what I could do. Same is true for the changes in the core. I did what I think is the clearest and most logical approach, you are free to adapt it to your vision. I have put already so much time into it, it works on my blog (which was the original goal) and I don't intend to change it back to something which I don't think as logical and straightforward. Syntax conventions like the if without brackets are another matter, there I did adapt. But I had a reason for what I did because I think that sometimes it is far more manageable in the long run to re-define a function than to stack a lot of exceptions on an existing one which becomes more and more cryptic as time goes by (the boolean thing is such a matter: instead of one case and several exceptions, it is much better to make a new, general one and define the special cases on demand). If you do that consequently from the start, you can crank down the complexity of a project a lot. |
|
One example of that general problem is for example that the SQL files and the SQL changes in the updater files are co-existing. Totally confusing. But the original intent - which is a good one - is that the SQL files are automatically and clearly separated by the different platforms, so everything which has different syntaxes should be done in the SQL files, even if it is possible to do in an updater script which additional checks. If there is only one version, the mysql.sql is executed (I did figure that out). Here, you just have to fix the *_sqlite.sql file and you are done. |
|
@stephanbrunker I took from above that you worked a lot on this and at that point hadn't been interested to finish it. Maybe that changed till now? :) I read again through this PR now. The open comments I made above will continue to block this PR - like the SQL pattern change, no one will come fix them. At some point this PR would simply be closed. Side-note: It looks right now that PHP 8 will force us to make a lot of code changes. A feature like this would profit a lot from being in master, so that we can fix it together. Keeping it as a separate branch will likely result in more work for you, even if it's just to keep it working for your blog. The feature is still a requested feature, but it should be possible to implement this with less breakage. Specifically, if you can transform this into a PR that:
Also, process-wise, please:
Then I would have another look at sqlite compatibility. At the very least I can propose a method that works with all db engines, maybe I could even submit a fix for sqlite specifically. |
|
It is difficult as I have to work 7 days a week, 12-14 hours a day to keep my company afloat. So atm I don't have any spare time.
But basically, the problem is that my style of work is not compatible with the s9y project as I have realized now as I do the crafting part of my buisiness. If I do something, I make it as clean and ordered as I can and I just cannot work otherwise. In the case of s9y, that meant that I needed to tidy up some scrambled and illogical parts of the core. My ambitions were not just the subscription feature, I also pulled the multilingual ability fully into the core - because there were already parts of it there and it just doesn't made sense to have it splitted in two locations. I did also a lot of work on the multilingual feature which was a long time stuck as a fork in additional_plugins until I did the merging into the core. Tidied also meant that I had to merge the existing comment subscription with the new blog, author and category subscription feature into one and I rewrote also the neccessary routing for it and reordered the database to make it as simple as possible. It would be a really pain to not touch the existing comment subscription and wedge the new features into the gaps. The result would be even more of a mess and a jumble of functions, spread over a multitude of database tables. I cannot work that way because you lose any resemblence of order. To make sending an html formatted email possible, I had to rewrite the sendMail function too. After that, i realized that all mails sent have a multilingual problem: they were sent in the language active at the time of sending, which in a multilingual environment is not the language the subscriber has subscribed to. I started to fix that by splitting the language files into a static part and a dynamic one containing the strings needed for mailing. As I worked on the language files anyway, I cleaned them up too. By that time, the first PR was stuck and I lost interest to finish that part. Same was true with the other points on my ToDo-list: make the cache working with the multilingual plugin (the cache is simply killed atm if multilingual is active), tidy the cache (as it is doubled in the serendipity array in the root part and the entryproperties part), and last: adding the feature to link an image to an entry like in some other blog systems. Essentially, if I do add something, I'll go the extra mile and make it in a way like it was intended from the beginning and not attached later. For that, I fixed all the references for every single function and structure I changed, in the core and in additional_plugins (with version check). If I stumble over an issue or something nasty while doing that, I fixed that too. That is how I experienced big software products: they simply change what they seem neccessary and as a user, you have to adapt or quit. And oftentimes, they don't even fix bugs until the next (paid) version if something went wrong. I have a lot of examples for that from different companies. I have bigger problems now, so I made peace with that piece of work. You can accept my style of work (of course I will fix every bug that will show itself, except for non-mysql databases). In that case, I'll even add the other issues from my ToDo as soon as I can. Or you can drop the PR. I can live with both. As it is, my personal blog runs with a version with most of these features added: stephanbrunker@bdf6b02 But I fear that the two forks have already diverged too much to be merged. And I stacked the new features on top of each other because I didn't expect the first one to be stuck while working several steps ahead. And I cannot un-stack and cripple the already written code, that seems to be a waste of time which I don't have as the latest commit just runs fine. |
|
Just to add an example why I work that way, which happened more than once: My colleagues mounted some hanging cupboads. Some time later, they realized that they forgot to add the cables for the lights. They decided to make it the fast way and wedged them behind the cupboard without pulling them down. Even more time later, it was my turn and several issues could occur: Either I wanted to pull up the loose - which didn't work because the cable was stuck. Or the cable was shortened. Either way, I had to do what they didn't: disassemble the whole set of cupboards - which was a lot more work because of the additional steps made in the meantime -, correct it to the right way and assemble the whole lot again. That meant that I had twice the trouble and it took thrice the time compared to do it right on the first try. Related to s9y that means in my opinion: if I have to touch something which could have made better from the beginning, I fix that. Because it may happen that you have to touch that part sometime in the future. If you make patchwork, you only make it even harder for your future self to work with the result because over time, it increasingly messes up. So, if the database tables are more logical with all the subscription data combined, I'll do that. If there is a function which has the logical name for what I want to do and does part of what I want, I rewrite that function. If the arguments are impractical in the order before with a mess of code to circumvent, I'll bring them into the logical order, even if I have to track all the prior usages. The only exception to that style of work is when something has hundreds of usages and simply cannot be fixed without rewriting major parts of the code, I have to accept that. But I am not happy with it. But that is me, and it is fully in your right to have a different opinion, which just means that I cannot contribute. |
The subscription feature is now running as an alpha version on my live blog www.intjblog.de and it looks very well. There are two version steps to alpha3 and alpha4 involved, the first was the subscription feature and the second one leading sections on the category and author pages. All the changes are in the NEWS file. So far, two things are untested: what happens to the links with the rewriteEngine on and the database changes for Postgres and SQLite.
On the testblog, I already have subscribers and a few new articles were sucessfully emailed to them.