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

Full Text Search #40

Closed
nticaric opened this issue Jul 14, 2016 · 36 comments
Closed

Full Text Search #40

nticaric opened this issue Jul 14, 2016 · 36 comments

Comments

@nticaric
Copy link
Contributor

@austintoddj have you considered adding full text search support? If you are interested, I can create a pull request by implementing TNTSearch

@austintoddj
Copy link
Owner

austintoddj commented Jul 14, 2016

I'd love to have a more fleshed out search. Right now, it is restricted to only posts and tags. There is much room for improvement! So yes, if you'd like to tackle this feature and send me a PR, I'd be more than happy to include it in the project. Let me know if you have any questions or need anything.

@nticaric
Copy link
Contributor Author

Ok great! The dependencies of TNTSearch are

  • PHP >= 5.5
  • PDO PHP Extension
  • SQLite PHP Extension
  • mbstring PHP Extension

So if we include it, canvas will also depend on it, are you ok with this?

@austintoddj
Copy link
Owner

Yes that's fine.

@nticaric
Copy link
Contributor Author

I guess we can close this one now. If someone experiences a bug with the search, feel free to assign the issue to me.

@austintoddj
Copy link
Owner

@nticaric Is this something that would be addressed in the config or .env file?
screen shot 2016-07-15 at 2 55 46 pm

@nticaric
Copy link
Contributor Author

Here's how the tntsearch config looks like

tntsearch

which means you need to have the DB_CONNECTION environment variable set

@austintoddj
Copy link
Owner

Two things. First, I get this error when trying to save a new post:

screen shot 2016-07-15 at 3 05 52 pm

Second: The PHPUnit tests significantly slowed down. The unit tests are using a SQLite database in memory to run tests rapidly. Any thoughts on these?

@austintoddj
Copy link
Owner

Also can't seem to pull any results from a search at all.

@nticaric
Copy link
Contributor Author

It seems like the folder where the database resides doesn't have write permissions. Can you please check if the folder and the index itself is writable. The default folder where the index resides is storage. The files are posts.index and tags.index

If the script failed to write to those indexes the search results will be empty

@austintoddj
Copy link
Owner

Yup I'll do that right away. FYI I just created a revert pull request, because I was hasty and just pulled everything in without testing myself. So I'm just reverting for a tiny bit while I do as much testing as I can and then I'll just have you send me a pull request again. Sorry for the confusion, I just should have been more thorough with my processes. But everything looks GREAT so far, and I'll gladly pull this in as soon as I run through it :)

@austintoddj austintoddj reopened this Jul 15, 2016
@nticaric
Copy link
Contributor Author

nticaric commented Jul 15, 2016

Ok, no problem. I think that the problem here were only the permissions.
About the unit test speed. The index files are updated each time an update/insert/delete is made to the posts or tags table. Since the tests do a lot of those actions, they are slowed down because the index resides on disk and writing to the disk is a lot slower than to memory.

We have several options for this:

  • we can remove the automatic updates/deletes/inserts and the user needs to run php artisan canvas:indexer if he wants to reindex
  • we remove the automatic updates/deletes/inserts and the user has an gui option for reindex
  • we can remove the automatic updates/deletes/inserts only for phpunit by doing an if env = testing

@austintoddj
Copy link
Owner

austintoddj commented Jul 15, 2016

Thanks for the explanation, it really helps. I think I like the 3rd option, since in a testing environment we are just looking for application functionality, and there won't really be a need to search for anything. So I think we'd be safe going that route.

As far as the permissions, is there anything we need to add to the readme.md for users on setting Canvas up, or will it just work?

Last, we'd need to update the .env.example with a DB_CONNECTION variable right?

@nticaric
Copy link
Contributor Author

I think you're already telling users to make the storage directory writable in the readme file.
Some systems, but rare ones, set an crazy umask for file creation that make the files readonly, so we could put a sentence that the two index files in the storage directory need to be writable.

Maybe it's better to change the tntsearch config file and default it to mysql env('DB_CONNECTION', 'mysql') because most users have mysql as their default database, and if not, either way, they have to add it manually to the .env file

@austintoddj
Copy link
Owner

Ok. Let's leave the current storage directory permissions statement in the readme alone for now. Could you go ahead and add the DB_CONNECTION to .env.example with a default of mysql? That way, it'll be pretty apparent for anyone not familiar with what's going on that their connection is set to mysql

@nticaric
Copy link
Contributor Author

Yes, I allready did that. Try to clone my fork and test it, and if everything goes smoothly, I'll send you a pull request, if not, we'll try to fix it ;)

https://github.com/nticaric/Canvas

@austintoddj
Copy link
Owner

Will do! I'll give it a go tonight. Thanks for working on this with me!

@nticaric
Copy link
Contributor Author

Hey @austintoddj, did you had any chance to look into this?

@austintoddj
Copy link
Owner

Have not yet, my apologies. Hoping to tonight however. Should see the merge soon! :)

@austintoddj
Copy link
Owner

Ok so I'm still getting the error Attempting to write a readonly database. Any thoughts @nticaric? Post index and tag index both have contents.

@nticaric
Copy link
Contributor Author

This must be due to your write permissions, the index file needs to be writable. Can you try to manually set it to ie. 777

@austintoddj
Copy link
Owner

@nticaric Ok awesome, think I've got it all working. You were right. Even though the storage folder is modified during the setup, the addition of the files at a later time means they aren't 777 like the directory was set. So it just means modifying the permissions again. Just making a couple more tests.

@austintoddj
Copy link
Owner

One other thing, looks like when I run the migrations and seed the database now, I get 2 duplicate tags attached to the 1 post. Have you run into this?

@nticaric
Copy link
Contributor Author

Hmm, this doesn't happen to me. Is it possible that you're running the seeder 2 times?

@austintoddj
Copy link
Owner

austintoddj commented Jul 21, 2016

Ah it's actually when I run canvas:install. Would you try running that command and see if you get it as well?

@nticaric
Copy link
Contributor Author

Each time the command canvas:install is run it adds a record to the post_tag_pivot table.
This is because the PostTagPivotTableSeeder doesn't truncate the table

@austintoddj
Copy link
Owner

Ah good catch! Do you want to add the truncate to your PR? Also, just trying to think of all the possibilities for error people could run into installing this. Is there anything in the setup process or steps that you can think of need to be addressed before we pull this in? If not, I think we're ready to go if you just send me a PR again.

@nticaric
Copy link
Contributor Author

Right now the only thing that I can think of are those permissions, everything else should be ok.

@austintoddj
Copy link
Owner

austintoddj commented Jul 21, 2016

Ok great. As soon as those tests are run, I'll merge it in. I'll re-arrange some steps in the readme and that should take care of any issues people could run into.

@austintoddj
Copy link
Owner

Nice work on this @nticaric. Love the feature, works great. Referenced TNTSearch in the readme as well.

@vhanla
Copy link

vhanla commented Sep 19, 2016

Every time I create or update a post it takes long time and it show FatalErrorException in TNTIndexer.php line 291 - Maximum execution time of 30 seconds exceeded.

@nticaric
Copy link
Contributor Author

Does the post contain some binary data, or base64 encoded, like inline images?

@vhanla
Copy link

vhanla commented Sep 19, 2016

Just the sample Hello World post

@nticaric
Copy link
Contributor Author

@vhanla can you check the post content to see how it looks in the database?

@austintoddj
Copy link
Owner

@nticaric Thanks for jumping in on this right away!

@vhanla
Copy link

vhanla commented Sep 19, 2016

@nticaric I just reinstalled a new empty project with composer create-project method, then accessed the admin and retried to edit the "Hello World" post, nothing really edited, just pressed 'Save' and it brought the same error.

Maybe it has to do with my current computer setup:
I'm using PHP 7.0.6 with Apache HttpServer 2.4 both 32bits running on Windows 10 x64.
PHP.ini extensions enabled : mbstring, pdo_sqlite, sqlite3.

Finally I changed PHPs script execution time to 130 seconds and it worked, but I had to wait almost those 130 seconds. I suppose it takes its time to do its job. Hopefully in my real server it will not take that long to post/update a post, since - locally - I'm using a "not really fast laptop" neither a Unix/Linux OS.

Anyway, thank you very much for your concern.

@austintoddj
Copy link
Owner

@vhanla Glad you got it working. And just for a comparison, when I run the same routine on a very basic production server, it only takes about 1-1.5 seconds to complete.

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

No branches or pull requests

3 participants