-
Notifications
You must be signed in to change notification settings - Fork 82
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
Support Memos for Visual FoxPro #118
base: master
Are you sure you want to change the base?
Conversation
Changes were made specifically for Visual FoxPro (TableType::VISUAL_FOXPRO), but it may work for other Versions too
src/Memo/MemoFactory.php
Outdated
@@ -25,7 +26,8 @@ public static function create(Table $table, EncoderInterface $encoder): ?MemoInt | |||
} | |||
$memoFilepath = $fileInfo['dirname'].DIRECTORY_SEPARATOR.$fileInfo['filename'].$memoExt; | |||
if (!file_exists($memoFilepath)) { | |||
return null; //todo create file? | |||
$memo_creator = MemoCreatorFactory::create($table); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the DB does not have memo fields, then a memo file will still be created. It is not right. TableWriter should be responsible for creating files. MemoFactory is part of TableReader. If memo file missing (but it should be), we need to pass this info to MemoFactory of use something like fixer-tool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, you're right. Without that line changed in MemoFactory
it is creating the Memo file. I'm not sure why I thought changing that was necessary 🤔
Anyway, currently TableWriter
appears to always create an empty Memo file if the TableType
supports Memos.
TableEditor
has a nice method for grabbing any set Memo Columns, but TableWriter
is its own Class that doesn't have access to TableEditor
's methods so we cannot currently use that. Would it be appropriate to move that method to a Trait so that both TableEditor
and TableWriter
can use it?
If TableWriter
had that method, we could have it check whether or not the Memo file needs to be created instead of always creating it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Yes, you are right, the library needs another refactoring. The problem with FoxPro is that I do not work with it and do not know all the subtleties.
There are not enough test cases in your PR. Can you write some?
@@ -22,6 +22,8 @@ public static function create(Table $table) | |||
case TableType::DBASE_7_MEMO: | |||
return new DBase7MemoCreator($table); | |||
//todo foxpro |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we delete this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The todo
? That I'm not sure, actually. I don't know anything about the other FoxPro versions and I've found that documentation is a bit scarce. I'm not sure if my changes here will work properly for the other versions :/
What tests specifically would you like to see? I'd be happy to write some, but without the refactoring necessary to be able to handle the Memo Field length, the Table Flag, and the issue I discovered in #117 I don't know how best to confirm that the file generation is occurring properly. |
TableWriter is handling creation of the Memo file, so it doesn't need to be done here. Referencing luads#118
The Backlist is meant to link to a Database (.DBC) file. Some applications (FoxPro specifically, I've noticed) get confused when it is initialized with non-0 values.
Changes were made specifically for Visual FoxPro (
TableType::VISUAL_FOXPRO
), but it may work for other Versions too.There are some minor issues with this PR, but they would likely require some refactoring of the library to handle nicely.
I've described the lingering issues below
Firstly, Visual FoxPro only expects 4 bytes for Memo fields, but
XBase\Header\Column\Validator\DBase\MemoValidator
will force this to 10 bytes no matter how you define the Column.php-xbase/src/Header/Column/Validator/DBase/MemoValidator.php
Lines 9 to 24 in e818e35
As a result, the Record Byte Length for any Tables that include Memo Fields will end up being incorrect. This will also impact the offset of the Column within the Record for any Fields that come after a Memo.
Given the way that the Memo field validation is being handled "globally" I'm not sure if there is a particularly good way to handle a different byte length only for specific DBase versions.
Secondly, while the
.FPT
file is written, technically the.DBF
file has no knowledge that it exists. The Table Flag will need to be set accordingly in the.DBF
file's header if Memo fields exist.I've corrected these two issues on my end by tweaking the
.DBF
file after initial creation with the following code:Lastly, one thing that may need to be tweaked is that the Memo file is created based on whether the current Table version supports Memos rather than if any Memo Columns exist or not. Currently, every time the Table is saved (meaning every time a Record is added) it will attempt to create a Memo file even if one isn't needed. I'm not sure if there's a good way around this though, since I know the parser can be set to only load certain Columns. If the Columns were restricted like this, it may not see the Memo columns in order to make this decision.